Closed
Bug 466269
Opened 16 years ago
Closed 16 years ago
Fall out from getTestCase numeric/NaN comparison bug.
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
People
(Reporter: bc, Assigned: bc)
References
Details
Attachments
(1 file, 1 obsolete file)
1.07 KB,
patch
|
mrbkap
:
review+
|
Details | Diff | Splinter Review |
Jim Blandy noticed a bug in getTestCaseResult which causes any comparison between a number and NaN to pass. See bug 461180. He and mrbkap are working on a replacement for getTestCase result, but the latest news was that there was a problem with the approach in attachment 349530 [details] [diff] [review] although I'm not sure what the problem was. Using a simple change to getTestCase result - if (expected_t != 'number' || (Math.abs(actual - expected) > 1E-10)) + if (expected_t != 'number' || typeof(actual) != typeof(expected) || + (Math.abs(actual - expected) > 1E-10)) to investigate which tests would fail due to the correction of the bug I found so far that the only failures are due to bugs in the tests which were hidden by the bug in getTestCaseResult: ecma_3/Date/15.9.4.3.js failures Date.UTC edge-case arguments.: date -Infinity : Expected value '31', Actual value 'NaN' Date.UTC edge-case arguments.: hours -Infinity : Expected value '0', Actual value 'NaN' Date.UTC edge-case arguments.: minutes -Infinity : Expected value '0', Actual value 'NaN' Date.UTC edge-case arguments.: seconds -Infinity : Expected value '0', Actual value 'NaN' Date.UTC edge-case arguments.: milliseconds -Infinity : Expected value '0', Actual value 'NaN' These are all errors in the test since the expected values were NaN which are tested earlier in the test. ecma/TypeConversion/9.3.1-3.js failures -"-infinity" = Infinity FAILED! NaN This is an error in the test since infinity is incorrect. mrbkap, this patch includes the brain-dead patch I first mentioned which I'm happy to drop. Can you just take a look and make sure the changes to the tests are ok? jimb, what was the problem with the other approach that you mentioned was a non-starter?
Attachment #349543 -
Flags: review?(mrbkap)
Updated•16 years ago
|
Attachment #349543 -
Flags: review?(mrbkap) → review+
Comment 1•16 years ago
|
||
Comment on attachment 349543 [details] [diff] [review] patch The test changes look fine to me.
Comment 3•16 years ago
|
||
For the hunk at js/tests/ecma_3/Date/15.9.4.3.js lines 187-214, are you sure we can delete those? I don't see anything else that checks the effect of passing -Infinity for the date. (This patch is only meant as a proposed alternative to that hunk; the second hunk looks fine to me. "FURTHER PATCH MUNGING CALLED FOR" ALERT.)
Attachment #349876 -
Flags: review?(mrbkap)
Updated•16 years ago
|
Attachment #349876 -
Flags: review?(mrbkap) → review+
Comment 4•16 years ago
|
||
Comment on attachment 349876 [details] [diff] [review] Alternative fix for ECMA-3 date tests This is probably better, yeah.
Comment 5•16 years ago
|
||
(In reply to comment #0) > jimb, what was the problem with the other approach that you mentioned was a > non-starter? It turns out that a lot of tests provide their expected values under the assumption that the coercing done by '==' (but not by '===') will take place. It would take a lot of changes across the test suite to make the whole test suite work with ===.
Assignee | ||
Updated•16 years ago
|
Attachment #349543 -
Attachment is obsolete: true
Assignee | ||
Comment 6•16 years ago
|
||
(In reply to comment #3) > Created an attachment (id=349876) [details] > Alternative fix for ECMA-3 date tests > > For the hunk at js/tests/ecma_3/Date/15.9.4.3.js lines 187-214, are you sure we > can delete those? I don't see anything else that checks the effect of passing > -Infinity for the date. > > (This patch is only meant as a proposed alternative to that hunk; the second > hunk looks fine to me. "FURTHER PATCH MUNGING CALLED FOR" ALERT.) true, I missed that the previous code was only for Infinity and not -Infinity. This is nit-picking to the extreme, but how about using the same pattern as the other tests? expect = true; actual = isNaN(new Date(Date.UTC(...)).getUTC...())
Assignee | ||
Comment 7•16 years ago
|
||
Jim, I've gone ahead and checked this in with the change in comment 6 and included it in an hg bundle to be pushed to m-c hopefully today. Checking in ecma_3/Date/15.9.4.3.js; /cvsroot/mozilla/js/tests/ecma_3/Date/15.9.4.3.js,v <-- 15.9.4.3.js new revision: 1.2; previous revision: 1.1 done Checking in ecma/TypeConversion/9.3.1-3.js; /cvsroot/mozilla/js/tests/ecma/TypeConversion/9.3.1-3.js,v <-- 9.3.1-3.js new revision: 1.9; previous revision: 1.8 done
Assignee: general → bclary
Status: NEW → RESOLVED
Closed: 16 years ago
Flags: in-testsuite-
Flags: in-litmus-
Resolution: --- → FIXED
Comment 8•16 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/3d6331e3ab27
You need to log in
before you can comment on or make changes to this bug.
Description
•