Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix and update tests #1788

Merged
merged 4 commits into from
Sep 26, 2018
Merged

Fix and update tests #1788

merged 4 commits into from
Sep 26, 2018

Conversation

anba
Copy link
Contributor

@anba anba commented Sep 24, 2018

d0cec12

  • Updates matchAll tests per latest spec proposal

9c704c1

  • Explicitly call String(...) to convert symbol value to a string, because the implicit ToString conversion throws a TypeError.
  • And fix the NaN test to expect a string, which may not necessarily be "NaN".

75e4fed

  • Split the tests which require the extra precision support into separate files, so that implementations don't have to skip the complete file.
  • This also helps to spot the negative zero sign issue in d51d6d3.

d51d6d3

  • Expect leading sign for negative zero per latest spec changes in ECMA-402.

… the expected NaN result is a (locale-dependent) string
This will allow implementations to test the rest of the number formatting tests,
even if the extra precision support isn't implemented.
@ljharb ljharb self-requested a review September 24, 2018 13:10
@leobalter
Copy link
Member

LGTM, @ljharb do you want to confirm the tests verify the intended changes in the proposal for matchAll??

@ljharb
Copy link
Member

ljharb commented Sep 24, 2018

@leobalter yes, please, that'd be great. i'm hoping to have that done by this evening; after i've stamped then feel free to merge <3

@@ -31,7 +30,8 @@ var o = {
get [Symbol.match]() {
++count;
return false;
}
},
flags: "",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be get flags() { } and increment a flagsCount, to ensure this is looked up?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this can be added as a follow up if you don't mind.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

certainly

@rwaldron rwaldron mentioned this pull request Sep 26, 2018
@rwaldron rwaldron merged commit 3a1bcae into tc39:master Sep 26, 2018
@anba anba deleted the test-bugs-sept2018 branch June 25, 2020 08:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants