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

Add tests for RegExp u flag #352

Merged
merged 1 commit into from
Jul 24, 2015
Merged

Add tests for RegExp u flag #352

merged 1 commit into from
Jul 24, 2015

Conversation

jugglinmike
Copy link
Contributor

I'd like to draw attention to two tests in particular since they both concern a very specific condition (for example, good ol' step 8.g.iv.5.d):

  • test/built-ins/RegExp/prototype/Symbol.match/u-lastindex.js
  • test/built-ins/RegExp/prototype/Symbol.replace/u-lastindex.js

Despite a careful reading, I'm still not convinced those test cases properly express the detail documented in their info/description. I decided I may have looked at it for too long and that some fresh perspective would help

@mathiasbynens Would you mind taking a look?


var r = /(𝌆)?/ug;
r[Symbol.match]('𝌆');
assert.sameValue(r.lastIndex, 2);
Copy link
Contributor

Choose a reason for hiding this comment

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

lastIndex is reset to 0 in 21.2.5.2.2 RegExpBuiltinExec, step 15.a.i when the loop in 21.2.5.6, step 8.g is executed the third (and last) time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, okay. So I think this behavior can't be externally verified through inspection of the lastIndex property. What about this instead:

test/built-ins/RegExp/prototype/Symbol.match/u-lastindex.js, renamed to test/built-ins/RegExp/prototype/Symbol.match/u-advance-after-empty.js

var match = /^|\udf06/ug[Symbol.match]('\ud834\udf06');

assert(match !== null);
assert.sameValue(match.length, 1);
assert.sameValue(match[0], '');

test/built-ins/RegExp/prototype/Symbol.replace/u-lastindex.js, renamed to test/built-ins/RegExp/prototype/Symbol.replace/u-advance-after-empty.js

var str = /^|\udf06/ug[Symbol.replace]('\ud834\udf06', 'XXX');
assert.sameValue(str, 'XXX\ud834\udf06');

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that should work. Alternatively it's also possible to use /\b/ and /\B/ assertions to execute those steps. Like in: https://github.com/anba/es6draft/blob/master/src/test/scripts/suite/regress/bug4159.js

@jugglinmike
Copy link
Contributor Author

I've fixed the bugs that @anba identified in a fixup commit

@goyakin
Copy link
Member

goyakin commented Jul 24, 2015

These are good tests @jugglinmike. It might be better to add the following tests to improve coverage:

  • Coercion of the unicode property to Boolean
  • length of the unicode get accessor function
  • Symbol.search

@jugglinmike
Copy link
Contributor Author

Sounds good to me. I've added tests for those three behaviors

@mathiasbynens
Copy link
Member

You could also add a test to see if running RegExp.prototype.unicode; (i.e. this is RegExp.prototype) throws.

@jugglinmike
Copy link
Contributor Author

Hey Mathias! That test actually describes to distinct expectations:

  1. RegExp.prototype is not a RegExp instance
  2. get RegExp.prototype.unicode requires the "this" value to have an [[OriginalFlags]] internal slot

I think it will be more clear (and maintainable) to test RegExp.prototype instanceof RegExp in one place and then only test for the second behavior generically (i.e. with any object missing the expected internal slot) in all other cases. The former is outside the scope of this pull request (though there is an open issue for it: #278), but this patch includes a test for the latter (https://github.com/tc39/test262/pull/352/files#diff-09b2f28b81fb4f5d4d84056cdced1ba6).

@bterlson
Copy link
Member

So I think this is good for 🍆?

@jugglinmike
Copy link
Contributor Author

Done and done

bterlson added a commit that referenced this pull request Jul 24, 2015
Add tests for RegExp `u` flag
@bterlson bterlson merged commit bdd84fb into tc39:master Jul 24, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants