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

Update tests for String.prototype.matchAll #1587

Merged
merged 1 commit into from Jun 8, 2018

Conversation

Projects
None yet
4 participants
@peterwmwong
Copy link
Contributor

peterwmwong commented Jun 5, 2018

As per spec changes (tc39/proposal-string-matchall#35), removed tests related to the removed IsRegExp call.

Also fix a misreference in features frontmatter.

/cc @ljharb

@ljharb
Copy link
Member

ljharb left a comment

Thanks!

@peterwmwong

This comment has been minimized.

Copy link
Contributor

peterwmwong commented Jun 5, 2018

@ljharb There's no rush, but I'm unclear what I should do next to get this merged. Are there others I need to add to review?

Thanks!

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Jun 5, 2018

@peterwmwong altho i have the commit bit, I don't think I should be the one to merge this :-) maybe @rwaldron or @leobalter could take a look?

kisg pushed a commit to paul99/v8mips that referenced this pull request Jun 6, 2018

[esnext] Update String.p.matchAll as per spec changes
As per (tc39/proposal-string-matchall#35), the
call to IsRegExp after CreateRegExp was removed and additional
checking was replaced by an Assert.

Updates to Test262 has been submitted:
tc39/test262#1587

Bug: v8:6890
Change-Id: I942b6846bb46cf85b1ea5566f9c19de7d2dbf03e
Reviewed-on: https://chromium-review.googlesource.com/1086419
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Reviewed-by: Mathias Bynens <mathias@chromium.org>
Commit-Queue: Peter Wong <peter.wm.wong@gmail.com>
Cr-Commit-Position: refs/heads/master@{#53539}
@leobalter

This comment has been minimized.

Copy link
Member

leobalter commented Jun 6, 2018

I wonder if we should expect a new behavior for the calls in the tests that are now removed here. In any case, I'd like to preserve the tests matching the same or modified behavior matching the current proposal spec. Consider we have implementations already observing these tests, now Test262 should also tell what's new to consider, right?

@peterwmwong

This comment has been minimized.

Copy link
Contributor

peterwmwong commented Jun 6, 2018

@leobalter The spec change is the removal of a call to IsRegExp (that was possibly user observable and what the removed tests verified). With it's removal, there's less observable behavior that can be tested and thus I simply removed the affected tests. Let me know if I misunderstood your question.

@ljharb correct me if I'm wrong, but I'm not sure what meaningful test could be written to verify that that particular IsRegExp is NOT called.

@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Jun 6, 2018

@peterwmwong you could keep the test that .calls with an object literal, but assert that its toString is called?

@peterwmwong

This comment has been minimized.

Copy link
Contributor

peterwmwong commented Jun 7, 2018

@ljharb, are you thinking something like these tests below?

string-tostring.js

// Copyright (C) 2018 Peter Wong. All rights reserved.
// This code is governed by the BSD license found in the LICENSE file.
/*---
esid: pending
description: String coercion of `string` argument
info: |
  RegExp.prototype [ @@matchAll ] ( string )
    [...]
    3. Return ? MatchAllIterator(R, string).
  MatchAllIterator ( R, O )
    1. Let S be ? ToString(O).
features: [Symbol.matchAll]
includes: [compareArray.js, compareIterator.js, regExpUtils.js]
---*/

var str = 'a*b';
var obj = {
  toString() {
    return str;
  }
};
var regexp = /\w/g;

assert.compareIterator(regexp[Symbol.matchAll](obj), [
  matchValidator(['a'], 0, str),
  matchValidator(['b'], 2, str)
]);

string-tostring-throws.js

// Copyright (C) 2018 Jordan Harband. All rights reserved.
// This code is governed by the BSD license found in the LICENSE file.
/*---
esid: pending
description: String coercion of string parameter
info: |
  RegExp.prototype [ @@matchAll ] ( string )
    [...]
    3. Return ? MatchAllIterator(R, string).
  MatchAllIterator ( R, O )
    1. Let S be ? ToString(O).
features: [Symbol.matchAll]
---*/

var obj = {
  valueOf() {
    $ERROR('This method should not be invoked.');
  },
  toString() {
    throw new Test262Error('toString invoked');
  }
};

assert.throws(Test262Error, function() {
  /toString value/[Symbol.matchAll](obj);
});
@ljharb

This comment has been minimized.

Copy link
Member

ljharb commented Jun 7, 2018

Yup, both of those look great :-)

@peterwmwong peterwmwong force-pushed the peterwmwong:update-matchall branch from f47c0d4 to ad6bcdf Jun 8, 2018

@peterwmwong

This comment has been minimized.

Copy link
Contributor

peterwmwong commented Jun 8, 2018

@ljharb @leobalter Huzzah! I have added a new test that will distinguish between implementations that have the spec update vs not:

https://github.com/peterwmwong/test262/blob/bbad9482e630aaa9d62716c067db29715d8f0cc1/test/built-ins/RegExp/prototype/Symbol.matchAll/isregexp-called-once.js

// Copyright (C) 2018 Peter Wong. All rights reserved.
// This code is governed by the BSD license found in the LICENSE file.
/*---
esid: pending
description: IsRegExp should only be called once
info: |
  RegExp.prototype [ @@matchAll ] ( string )
    [...]
    3. Return ? MatchAllIterator(R, string).

  MatchAllIterator ( R, O )
    [...]
    2. If ? IsRegExp(R) is true, then
      [...]
    3. Else,
      a. Let flags be "g".
      b. Let matcher be ? RegExpCreate(R, flags).
features: [Symbol.match, Symbol.matchAll]
---*/

var internalCount = 0;
Object.defineProperty(RegExp.prototype, Symbol.match, {
  get: function() {
    ++internalCount;
    return true;
  }
});

var count = 0;
var o = {
  get [Symbol.match]() {
    ++count;
    return false;
  }
};

RegExp.prototype[Symbol.matchAll].call(o, '1');

assert.sameValue(0, internalCount);
assert.sameValue(1, count);

I verified the test fails without my latest CL to V8: https://chromium-review.googlesource.com/c/v8/v8/+/1086419

I have preemptively updated my commit with this new test.
What do you think?

Update tests for String.prototype.matchAll
As per spec changes (tc39/proposal-string-matchall#35), removed tests related to the removed IsRegExp call.
To prevent older implementations (not observing spec change) from passing, added a new test to verify the reduced number of observable calls to IsRegExp.

Also fix a misreference in `features` frontmatter.

@peterwmwong peterwmwong force-pushed the peterwmwong:update-matchall branch from ad6bcdf to bbad948 Jun 8, 2018

@ljharb

ljharb approved these changes Jun 8, 2018

@rwaldron rwaldron merged commit f90a52b into tc39:master Jun 8, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@leobalter

This comment has been minimized.

Copy link
Member

leobalter commented Jun 8, 2018

Thanks for the follow ups, @peterwmwong!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment