Correct invalid tests for the Date constructor #573
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The millisecond representation of a given Date instance is dependent on the local system's time zone settings. In order to pass consistently across contexts, tests for this value must take the system configuration into account. Introduce a test harness utility function to encapsulate these concerns. Re-use this function across all test files that assert the exact millisecond representation of Date instances.
jugglinmike
changed the title
jugglinmike
force-pushed
the
fix-date-tests
branch
from
beb47e2 to
92a1824
Compare
| $ERROR('Expected error, but no error was thrown.'); | ||
| } else if (thrown.constructor !== Test262Error) { | ||
| $ERROR('Expected error of type Test262Error.'); | ||
| } |
There was a problem hiding this comment.
why don't we use assert.throws here? I'm assuming you're avoiding the use of other harness, is that right?
|
LGTM, my comments above shouldn't block anything. |
|
@littledan I'm +1 to merge this. Would you confirm this fixes #528? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The millisecond representation of a given Date instance is dependent on
the local system's time zone settings. In order to pass consistently
across contexts, tests for this value must take the system configuration
into account.
Introduce a test harness utility function to encapsulate these concerns.
Re-use this function across all test files that assert the exact
millisecond representation of Date instances.
@littledan As noted in the original BugZilla report, this approach complicates
the tests, but it doesn't quite depend on the semantics under test, so it
should be "safe."
That said, another solution occurred to me while writing this: we could make a
specific time zone (say, Greenwich Mean Time) a formal requirement to executing
Test262. This seems like the same class of problems I am trying to address in
gh-508. That is: limiting runtime variability through requirements
specification.
...But maybe it's a little too presumptuous to ask consumers to re-configure
their system clocks before running the tests. It certainly seems like overkill
for these 6 files, but doing so would let us forget about this detail entirely
(instead of carrying around another helper function and remembering to use it).
Resolves gh-528 (and https://bugs.ecmascript.org/show_bug.cgi?id=293 before it)