466269 - Fall out from getTestCase numeric/NaN comparison bug.

Status: RESOLVED FIXED

Description

[Bob Clary [:bc:]]

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?

Comment 1

[Blake Kaplan (:mrbkap)]

Comment on attachment 349543 [details] [diff] [review]
patch

The test changes look fine to me.

Comment 3

[Jim Blandy :jimb]

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.)

Comment 4

[Blake Kaplan (:mrbkap)]

Comment on attachment 349876 [details] [diff] [review]
Alternative fix for ECMA-3 date tests

This is probably better, yeah.

Comment 5

[Jim Blandy :jimb]

(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 ===.

Comment 6

[Bob Clary [:bc:]]

(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...())

Comment 7

[Bob Clary [:bc:]]

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

Comment 8

[Shawn Wilsher :sdwilsh]

http://hg.mozilla.org/mozilla-central/rev/3d6331e3ab27