Closed
Bug 466269
Opened 17 years ago
Closed 17 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•17 years ago
|
Attachment #349543 -
Flags: review?(mrbkap) → review+
Comment 1•17 years ago
|
||
Comment on attachment 349543 [details] [diff] [review]
patch
The test changes look fine to me.
Comment 3•17 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•17 years ago
|
Attachment #349876 -
Flags: review?(mrbkap) → review+
Comment 4•17 years ago
|
||
Comment on attachment 349876 [details] [diff] [review]
Alternative fix for ECMA-3 date tests
This is probably better, yeah.
Comment 5•17 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•17 years ago
|
Attachment #349543 -
Attachment is obsolete: true
Assignee | ||
Comment 6•17 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•17 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: 17 years ago
Flags: in-testsuite-
Flags: in-litmus-
Resolution: --- → FIXED
Comment 8•17 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•