Increase test coverage: Array.prototype.splice #2682

matedabis · 2019-01-08T11:04:01Z

Branch coverage:
Before: 52/72
After: 72/72

Also found an opportunity to optimize, that "else if" branch is not needed,
should be replaced with an "else".

JerryScript-DCO-1.0-Signed-off-by: Mate Dabis [email protected]

LaszloLango · 2019-01-09T13:39:26Z

tests/jerry/array-prototype-splice.js

+  assert (e instanceof ReferenceError);
+}
+
+try{


Missing space before '{'
There are many minor style issues like this in the added test file. Please always put a space before '{', put semicolon after statements, do not write space before '(' in function calls (e.g.: assert(false);, etc.)

Check your code and fix these issues.

matedabis · 2019-01-09T15:05:13Z

@LaszloLango
Corrected the mistakes mentioned.

LaszloLango

One thing left, good to me after that.

LaszloLango · 2019-01-09T15:08:27Z

tests/jerry/array-prototype-splice.js

+try {
+  Array.prototype.splice.call(undefined);
+  assert(false);
+} catch(e) {


Missing space: } catch (e) {
Please fix it in the whole file.

matedabis · 2019-01-10T07:45:32Z

Fixed

LaszloLango

LGTM

rerobika

The branch coverage improvement is OK, but test the expected behavior as well like you did in #2685 or #2688.

rerobika · 2019-01-11T11:13:39Z

tests/jerry/array-prototype-splice.js

+  obj = {get: f, valueOf : f, toString: f};
+  arr = [1, 2, obj, 4, 5];
+  Object.defineProperty(arr, '2',{ 'get' : f } );
+  print(arr);


print ??? Why is it even needed?

rerobika · 2019-01-11T11:14:19Z

tests/jerry/array-prototype-splice.js

+  var a = [1, 5, 6, 7, 8, 5];
+  Object.defineProperty(a, '1', { 'get' : function() { throw new ReferenceError("foo"); } });
+  Object.defineProperty(a, '0', { 'get' : function() { throw new ReferenceError("foo"); } });
+  Object.defineProperty(a, '2', { 'get' : function() { throw new ReferenceError("foo"); } });


Use different error messages, and assert in the catch block which error has been thrown.

tests/jerry/array-prototype-splice.js

rerobika · 2019-01-11T11:21:25Z

Also I'd rather call the else if to else replacement to optimization than mistake correction in the commit message.

matedabis · 2019-01-11T14:22:09Z

@rerobika
That print is needed to make an ECMA_TRY_CATCH fail and acces that branch in line 1346.
I corrected the other issues.

matedabis · 2019-01-14T07:35:17Z

Corrected.

akosthekiss · 2019-01-14T17:26:57Z

@matedabis Could you please give details similar to those requested at #2674 (comment) . I.e., which source file(s) or source file entity(-ies) (function(s)) are investigated, and what is the coverage result when all test suites of the project are executed (i.e., what is the actual baseline)?

matedabis · 2019-01-15T07:55:35Z

@akosthekiss
The function we are testing: ecma_builtin_array_prototype_object_splice
The test file: /tests/jerry/array-prototype-splice.js
I dont know yet the result if all test suites are executed. By all test suites of the project do you mean all the tests in run-tests.py or all the .js files in the tests folder, or both?

akosthekiss · 2019-01-15T10:04:17Z

I almost forgot about unit tests and various configurations. So, I think combining the coverage results of all run-tests.py executions is the best way.

akosthekiss

I'd like to have this on hold until validity questions are sorted out at #2674 .

matedabis · 2019-01-17T12:28:42Z

@akosthekiss
Now the script is modified and is able to run and collect coverage information from --jerry-test-suite --test262 --unittests --jerry-tests tests. Now we are working on improving the branch coverage by reaching those branches that neither of the tests cover.
https://github.com/matedabis/jerryscript/blob/gcov_coverage_tester/tests/gcov-tests/gcovtester.py

rerobika · 2019-01-18T17:12:50Z

tests/jerry/array-prototype-splice.js

+}
+
+var a = {length : 13};
+Array.prototype.splice.call(a, function () {delete a});


Spaces around the function body's expression statement.

rerobika · 2019-01-18T17:13:50Z

tests/jerry/array-prototype-splice.js

+} catch (e) {}
+
+try {
+  f = function() { for(var i = 0; i < arr.length; i++) { delete arr[i]} };


Break it into multiple lines.

+1. (Plus: there are multiple similar constructs in the code; apply a consistent style everywhere)

rerobika · 2019-01-18T17:14:32Z

tests/jerry/array-prototype-splice.js

+  Object.defineProperty(arr, '2', { 'get' : f });
+  Object.defineProperty(arr, '3', { 'get' : f });
+  Object.defineProperty(arr, '4', { 'get' : f });
+  Object.defineProperty(arr, '5', { 'get' : f });


How about using a for loop for these 6 property definitions?

rerobika · 2019-01-18T17:18:35Z

tests/jerry/array-prototype-splice.js

+  assert(e.message == "4");
+}
+
+try{


Could you please add a short description for each new testcase? After this point the test cases are slightly complicated, it would be good to know what is happening.

matedabis · 2019-01-21T09:14:44Z

I updated the patch according to the review.

akosthekiss · 2019-01-21T13:09:50Z

tests/jerry/array-prototype-splice.js

+  assert(e instanceof TypeError);
+}
+
+// Checking behaior when elements are getting deleted the same way, but there are more elements and arguments


typo: behaior

akosthekiss · 2019-01-21T13:11:13Z

tests/jerry/array-prototype-splice.js

+  Object.defineProperty(arr, '8', { 'get' : f });
+  Object.defineProperty(arr, '9', { 'get' : f2 });
+  Object.defineProperty(arr, '10', { 'get' : f3 });
+  arr.splice(1, 7, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5, 5);


Is this really the simplest use case to test a feature of the engine? Is a 24 element array, 7 manually defined properties, and a 21 element splice needed to cover some path?

akosthekiss · 2019-01-21T13:14:02Z

tests/jerry/array-prototype-splice.js

+  assert(e instanceof TypeError);
+}
+
+// Checking behavior when first 3 elements throw error


Which code path needs the first/last 3 elements to be handled specially?

akosthekiss · 2019-01-21T13:14:55Z

tests/jerry/array-prototype-splice.js

+
+// Checking behavior when last 3 elements throw error
+try {
+  f = function() {  for(var i=0; i<arr.length; i++) { delete arr[i]} };


style: spacing (too many + missing). also: missing ; from after delete statement.

akosthekiss · 2019-01-21T13:15:08Z

tests/jerry/array-prototype-splice.js

+
+// Checking behavior when the last element gets deleted
+try {
+  f = function () { delete arr[23];};


style: spacing

akosthekiss · 2019-01-21T13:15:23Z

tests/jerry/array-prototype-splice.js

+  f = function () { delete arr[23];};
+  arr = [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24];
+  delete arr[23];
+  Object.defineProperty(arr, '23',{ 'get' : f });


style: spacing

akosthekiss · 2019-01-21T13:17:36Z

tests/jerry/array-prototype-splice.js

+} catch (e) {}
+
+try {
+  f = function() { for(var i = 0; i < arr.length; i++) { delete arr[i]} };


+1. (Plus: there are multiple similar constructs in the code; apply a consistent style everywhere)

akosthekiss · 2019-01-21T13:18:00Z

tests/jerry/array-prototype-splice.js

+  f = function() { for(var i = 0; i < arr.length; i++) { delete arr[i]} };
+  f1 = function() { delete arr[2];};
+  f2 = function() { delete arr[3];};
+  f3 = function() { delete arr[4];};


style: spacing

akosthekiss · 2019-01-21T13:18:25Z

tests/jerry/array-prototype-splice.js

+
+// Checking behaior when elements are getting deleted the same way, but there are more elements and arguments
+try {
+  f = function() { for(var i=0; i<arr.length; i++) { delete arr[i]} };


style: spacing, semicolon

rerobika · 2019-01-22T11:14:48Z

tests/jerry/array-prototype-splice.js

+        delete arr[3];
+        arr.length = 13;
+        Object.defineProperty(arr, '5', function() { });
+      };


This style has became even worse than it was before. Please use the following format:

f = function() { delete arr[3]; arr.length = 13; Object.defineProperty(arr, '5', function() { }); };

rerobika · 2019-01-22T11:17:18Z

tests/jerry/array-prototype-splice.js

+  arr = [1, 2, obj, 4, 5];
+  Object.defineProperty(arr, '2',{ 'get' : f } );
+  for(var i = 0; i < arr.length; i++)
+  {


Move the open curly brace up:

for (var i = 0; i < arr.length; i++) { //...body }

matedabis · 2019-01-23T14:46:34Z

Updated the patch:

Added comments for each case containing which part of the ES 5.1 it tests.
Sorted new tests in ascending order
Corrected the patch according to the reviews. I made some test cases simpler
Ran the test code in Gecko, V8, SpiderMonkey engines and got the same result.

Added new test cases to improve branch coverage in Array.prototype routines. The following script is made for testing branch coverage with all the test suites (--jerry-test-suite --test262 --unittests --jerry-tests), or with only one .js file. https://github.com/matedabis/jerryscript/blob/gcov_coverage_tester/tests/gcov-tests/gcovtester.py While measuring the branch coverage we dont count JERRY_ASSERT s. The results are measured by running all the test scripts, with the modifications in the PRs. Branch coverage including jerryscript-project#2682 and jerryscript-project#2674: -before: 492 / 578 -after: 570 / 574 There are 28 functions in ecma-builtin-arraybuffer-prototype.c, we hit 14 from them. The other 14 functions are either already covered, or we could not improve the coverage of it. Co-authored-by: Mate Dabis [email protected] JerryScript-DCO-1.0-Signed-off-by: Mate Dabis [email protected] JerryScript-DCO-1.0-Signed-off-by: Csaba Repasi [email protected]

Added new test cases to improve branch coverage in Array.prototype routines. The following script is made for testing branch coverage with all the test suites (--jerry-test-suite --test262 --unittests --jerry-tests), or with only one .js file. https://github.com/matedabis/jerryscript/blob/gcov_coverage_tester/tests/gcov-tests/gcovtester.py While measuring the branch coverage we dont count JERRY_ASSERT s. The results are measured by running all the test scripts, with the modifications in the PRs. Branch coverage including jerryscript-project#2682 and jerryscript-project#2674: -before: 492 / 578 -after: 570 / 574 There are 28 functions in ecma-builtin-array-prototype.c, we hit 14 from them. The other 14 functions are either already covered, or we could not improve the coverage of it. Co-authored-by: Mate Dabis [email protected] JerryScript-DCO-1.0-Signed-off-by: Mate Dabis [email protected] JerryScript-DCO-1.0-Signed-off-by: Csaba Repasi [email protected]

matedabis · 2019-02-08T11:44:54Z

Summary:
The following script is made for testing branch coverage with all the test suites (--jerry-test-suite --test262 --unittests --jerry-tests), or with only one .js file.

https://github.com/matedabis/jerryscript/blob/gcov_coverage_tester/tests/gcov-tests/gcovtester.py

While measuring the branch coverage we don't count JERRY_ASSERT s.

Branch coverage (covered branch/all branch):
Before: 52/72
After: 72/72

Also found an opportunity to optimize, that "else if" branch is not needed,
should be replaced with an "else".

Branch coverage: Before: 52/72 After: 72/72 Also found opportunity for optimization, that "else if" branch is not needed, should be replaced with an "else". JerryScript-DCO-1.0-Signed-off-by: Mate Dabis [email protected]

Added new test cases to improve branch coverage in Array.prototype routines. The following script is made for testing branch coverage with all the test suites (--jerry-test-suite --test262 --unittests --jerry-tests), or with only one .js file. https://github.com/matedabis/jerryscript/blob/gcov_coverage_tester/tests/gcov-tests/gcovtester.py While measuring the branch coverage we dont count JERRY_ASSERT s. The results are measured by running all the test scripts, with the modifications in the PRs. Branch coverage including pando-project#2682 and pando-project#2674: -before: 401 / 478 -after: 474 / 478 There are 28 functions in ecma-builtin-array-prototype.c, we hit 14 from them. The other 14 functions are either already covered, or we could not improve the coverage of it. More information on the link below: https://gist.github.com/matedabis/d7b9fc0690aa2f4be6aa160fdf482e0e Co-authored-by: Mate Dabis [email protected] JerryScript-DCO-1.0-Signed-off-by: Mate Dabis [email protected] JerryScript-DCO-1.0-Signed-off-by: Csaba Repasi [email protected] JerryScript-DCO-1.0-Signed-off-by: Mate Dabis [email protected]

Added new test cases to improve branch coverage in Array.prototype routines. The following script is made for testing branch coverage with all the test suites (--jerry-test-suite --test262 --unittests --jerry-tests), or with only one .js file. https://github.com/matedabis/jerryscript/blob/gcov_coverage_tester/tests/gcov-tests/gcovtester.py While measuring the branch coverage we dont count JERRY_ASSERT s. The results are measured by running all the test scripts, with the modifications in the PRs. Branch coverage including pando-project#2682 and pando-project#2674: -before: 401 / 478 -after: 474 / 478 There are 28 functions in ecma-builtin-array-prototype.c, we hit 14 from them. The other 14 functions are either already covered, or we could not improve the coverage of it. More information about the coverage improvement and the branches not reached: https://gist.github.com/matedabis/d7b9fc0690aa2f4be6aa160fdf482e0e Co-authored-by: Csaba Repasi [email protected] JerryScript-DCO-1.0-Signed-off-by: Csaba Repasi [email protected] JerryScript-DCO-1.0-Signed-off-by: Mate Dabis [email protected]

Added new test cases to improve branch coverage in Array.prototype routines. The following script is made for testing branch coverage with all the test suites (--jerry-test-suite --test262 --unittests --jerry-tests), or with only one .js file. https://github.com/matedabis/jerryscript/blob/gcov_coverage_tester/tests/gcov-tests/gcovtester.py While measuring the branch coverage we dont count JERRY_ASSERT s. The results are measured by running all the test scripts, with the modifications in the PRs. Branch coverage including pando-project#2682 and pando-project#2674: -before: 401 / 478 -after: 474 / 478 There are 28 functions in ecma-builtin-array-prototype.c, we hit 14 from them. The other 14 functions are either already covered, or we could not improve the coverage of it. More information about the coverage improvement and the branches not reached: https://gist.github.com/matedabis/d7b9fc0690aa2f4be6aa160fdf482e0e While improving the coverage we found an unnecessary condition check, which can not be false in any cases. Co-authored-by: Csaba Repasi [email protected] JerryScript-DCO-1.0-Signed-off-by: Csaba Repasi [email protected] JerryScript-DCO-1.0-Signed-off-by: Mate Dabis [email protected]

Added new test cases to improve branch coverage in Array.prototype routines. The following script is made for testing branch coverage with all the test suites (--jerry-test-suite --test262 --unittests --jerry-tests), or with only one .js file. https://github.com/matedabis/jerryscript/blob/gcov_coverage_tester/tests/gcov-tests/gcovtester.py While measuring the branch coverage we dont count JERRY_ASSERT s. The results are measured by running all the test scripts, with the modifications in the PRs. Branch coverage including pando-project#2682 and pando-project#2674: -before: 399 / 476 -after: 472 / 476 There are 28 functions in ecma-builtin-array-prototype.c, we hit 14 from them. The other 14 functions are either already covered, or we could not improve the coverage of it. More information about the coverage improvement and the branches not reached: https://gist.github.com/matedabis/d7b9fc0690aa2f4be6aa160fdf482e0e While improving the coverage we found an unnecessary condition check, which can not be false in any cases. Co-authored-by: Csaba Repasi [email protected] JerryScript-DCO-1.0-Signed-off-by: Csaba Repasi [email protected] JerryScript-DCO-1.0-Signed-off-by: Mate Dabis [email protected]

matedabis · 2019-03-13T08:57:53Z

Closing PR because of moving into a unified PR.

Added new test cases to improve branch coverage in Array.prototype routines. The following script is made for testing branch coverage with all the test suites (--jerry-test-suite --test262 --unittests --jerry-tests), or with only one .js file. https://github.com/matedabis/jerryscript/blob/gcov_coverage_tester/tests/gcov-tests/gcovtester.py While measuring the branch coverage we dont count JERRY_ASSERT s. The results are measured by running all the test scripts, with the modifications in the PRs. Branch coverage including pando-project#2682 and pando-project#2674: -before: 399 / 476 -after: 472 / 476 There are 28 functions in ecma-builtin-array-prototype.c, we hit 14 from them. The other 14 functions are either already covered, or we could not improve the coverage of it. More information about the coverage improvement and the branches not reached: https://gist.github.com/matedabis/d7b9fc0690aa2f4be6aa160fdf482e0e While improving the coverage we found an unnecessary condition check, which can not be false in any cases. Co-authored-by: Csaba Repasi [email protected] JerryScript-DCO-1.0-Signed-off-by: Csaba Repasi [email protected] JerryScript-DCO-1.0-Signed-off-by: Mate Dabis [email protected]

matedabis changed the title ~~Improvement in branch coverage of ecma_builtin_array_prototype_object_splice~~ Increase test coverage: ecma_builtin_array_prototype_object_splice Jan 8, 2019

matedabis changed the title ~~Increase test coverage: ecma_builtin_array_prototype_object_splice~~ Increase test coverage: Array.prototype.splice Jan 8, 2019

matedabis force-pushed the ecma_builtin_array_prototype_object_splice_coverage branch from e5f8e7b to c108b17 Compare January 8, 2019 11:17

LaszloLango requested changes Jan 9, 2019

View reviewed changes

matedabis force-pushed the ecma_builtin_array_prototype_object_splice_coverage branch 2 times, most recently from 2ef0f11 to 8f29248 Compare January 9, 2019 15:03

LaszloLango requested changes Jan 9, 2019

View reviewed changes

matedabis force-pushed the ecma_builtin_array_prototype_object_splice_coverage branch from 8f29248 to 54074ec Compare January 10, 2019 07:45

LaszloLango approved these changes Jan 10, 2019

View reviewed changes

rerobika requested changes Jan 11, 2019

View reviewed changes

matedabis force-pushed the ecma_builtin_array_prototype_object_splice_coverage branch from 54074ec to 96f1bf1 Compare January 11, 2019 14:15

matedabis force-pushed the ecma_builtin_array_prototype_object_splice_coverage branch from 96f1bf1 to 4179d55 Compare January 14, 2019 07:25

akosthekiss requested changes Jan 17, 2019

View reviewed changes

rerobika requested changes Jan 18, 2019

View reviewed changes

rerobika added the test Related to testing label Jan 19, 2019

matedabis force-pushed the ecma_builtin_array_prototype_object_splice_coverage branch from 4179d55 to 05c044f Compare January 21, 2019 09:08

matedabis mentioned this pull request Jan 21, 2019

Increase branch coverage: Array.prototype routines #2721

Closed

akosthekiss requested changes Jan 21, 2019

View reviewed changes

matedabis force-pushed the ecma_builtin_array_prototype_object_splice_coverage branch 2 times, most recently from e4f635b to 676a3d1 Compare January 21, 2019 13:42

matedabis force-pushed the ecma_builtin_array_prototype_object_splice_coverage branch from 676a3d1 to 4abd6ac Compare January 22, 2019 10:51

rerobika requested changes Jan 22, 2019

View reviewed changes

matedabis force-pushed the ecma_builtin_array_prototype_object_splice_coverage branch 2 times, most recently from 11fe246 to 44c2b34 Compare January 23, 2019 14:45

matedabis force-pushed the ecma_builtin_array_prototype_object_splice_coverage branch 5 times, most recently from dc682a3 to fee86e5 Compare January 30, 2019 08:50

Increase test coverage: Array.prototype.splice

835b1f9

Branch coverage: Before: 52/72 After: 72/72 Also found opportunity for optimization, that "else if" branch is not needed, should be replaced with an "else". JerryScript-DCO-1.0-Signed-off-by: Mate Dabis [email protected]

matedabis force-pushed the ecma_builtin_array_prototype_object_splice_coverage branch from fee86e5 to 835b1f9 Compare February 20, 2019 09:59

matedabis closed this Mar 13, 2019

zherczeg mentioned this pull request Mar 19, 2019

Nominating Robert Fancsik (@rerobika) to become a JerryScript reviewer #2799

Closed

Increase test coverage: Array.prototype.splice #2682

Increase test coverage: Array.prototype.splice #2682

Uh oh!

Conversation

matedabis commented Jan 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matedabis commented Jan 9, 2019

Uh oh!

LaszloLango left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matedabis commented Jan 10, 2019

Uh oh!

LaszloLango left a comment

Choose a reason for hiding this comment

Uh oh!

rerobika left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rerobika commented Jan 11, 2019

Uh oh!

matedabis commented Jan 11, 2019

Uh oh!

matedabis commented Jan 14, 2019

Uh oh!

akosthekiss commented Jan 14, 2019

Uh oh!

matedabis commented Jan 15, 2019

Uh oh!

akosthekiss commented Jan 15, 2019

Uh oh!

akosthekiss left a comment

Choose a reason for hiding this comment

Uh oh!

matedabis commented Jan 17, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matedabis commented Jan 21, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matedabis commented Jan 8, 2019 •

edited

Loading

matedabis commented Feb 8, 2019 •

edited

Loading