Fall out from getTestCase numeric/NaN comparison bug.

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: bc, Assigned: bc)

Tracking

Trunk
Points:
---
Bug Flags:
in-testsuite -
in-litmus -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

10 years ago
Created attachment 349543 [details] [diff] [review]
patch

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

10 years ago
Attachment #349543 - Flags: review?(mrbkap) → review+
Comment on attachment 349543 [details] [diff] [review]
patch

The test changes look fine to me.

Updated

10 years ago
Duplicate of this bug: 466558

Comment 3

10 years ago
Created attachment 349876 [details] [diff] [review]
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.)
Attachment #349876 - Flags: review?(mrbkap)

Updated

10 years ago
Attachment #349876 - Flags: review?(mrbkap) → review+
Comment on attachment 349876 [details] [diff] [review]
Alternative fix for ECMA-3 date tests

This is probably better, yeah.

Comment 5

10 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

10 years ago
Attachment #349543 - Attachment is obsolete: true
(Assignee)

Comment 6

10 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

10 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
Last Resolved: 10 years ago
Flags: in-testsuite-
Flags: in-litmus-
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.