Increase test coverage: Array.prototype.toLocaleString #2716
Conversation
| Array.prototype.toLocaleString.call(obj); | ||
| assert(false); | ||
| } catch (e) { | ||
| assert(e instanceof ReferenceError); |
There was a problem hiding this comment.
Please check the message as well! (And use unique messages)
There was a problem hiding this comment.
The message is not checked. Why is this marked as resolved?
There was a problem hiding this comment.
@akosthekiss Probably that was a miss click, I've just unresolved it.
There was a problem hiding this comment.
The thrown error and the one in the function are not the same ReferenceErrors, so as I check the message, I get assertion fail.
There was a problem hiding this comment.
Something is not right then. Q1: Why set the error message then, if it cannot be checked? Q2: Does the error object get replaced somewhere by the builtin implementation? Q3: If yes, why? Q4: If yes, is it necessary to throw a reference error within the anonymous function or any kind of error can be thrown?
There was a problem hiding this comment.
Something is a bit fishy there.
try {
var obj = {};
Object.defineProperty(obj, 'length', { 'get' : function () { throw new ReferenceError("foo"); } });
Array.prototype.toLocaleString.call(obj);
assert(false);
} catch (e) {
assert(e instanceof ReferenceError);
assert (e.message == "foo");
}The suggested assertion for the message is correct in all engines that I've tested (V8, Spidermonkey, JSC) even in Jerry as well. Could you please give more detailed description about why this assert is failing?
There was a problem hiding this comment.
Sorry I made a typo, that is why it did not work.
| assert(e instanceof ReferenceError); | ||
| } | ||
|
|
||
| // Checking behavior when length is an object, wichs valueOf throws error |
matedabis
force-pushed
the
to_locale_string
branch
from
c15e190 to
d7a9fc9
Compare
|
Corrected according to the reviews. |
| @@ -54,3 +54,34 @@ try { | |||
| assert (e.message === "foo"); | |||
| assert (e instanceof ReferenceError); | |||
| } | |||
|
|
|||
| // Checking behavior when null is passed to the function | |||
There was a problem hiding this comment.
This description is a bit misleading. I'd suggest the same as in #2717.
matedabis
force-pushed
the
to_locale_string
branch
from
d7a9fc9 to
f833585
Compare
|
I updated the patch according to the review. |
| Array.prototype.toLocaleString.call(obj); | ||
| assert(false); | ||
| } catch (e) { | ||
| assert(e instanceof ReferenceError); |
There was a problem hiding this comment.
The message is not checked. Why is this marked as resolved?
| assert(e instanceof ReferenceError); | ||
| } | ||
|
|
||
| // Checking behavior when length is an object, whichs valueOf throws error |
There was a problem hiding this comment.
typo: whichs --> which has a valueOf method that throws an error
matedabis
force-pushed
the
to_locale_string
branch
from
f833585 to
2665841
Compare
|
Corrected the patch according to the review. |
matedabis
force-pushed
the
to_locale_string
branch
from
2665841 to
97d13e1
Compare
|
Updated the patch:
|
matedabis
force-pushed
the
to_locale_string
branch
from
97d13e1 to
74bd487
Compare
|
Corrected the issue above. |
Branch coverage: Before: 15/18 After: 18/18 JerryScript-DCO-1.0-Signed-off-by: Mate Dabis [email protected]
matedabis
force-pushed
the
to_locale_string
branch
from
74bd487 to
38b8655
Compare
|
Changed comment style. |
|
Closing PR because of moving to another branch. |
The coverage is measured by the following script, using
--jerry-test-suite --test262 --unittests --jerry-tests:https://github.com/matedabis/jerryscript/blob/gcov_coverage_tester/tests/gcov-tests/gcovtester.py
Branch coverage:
Before: 15/18
After: 18/18
JerryScript-DCO-1.0-Signed-off-by: Mate Dabis [email protected]