Added/updated tests for matchAll by peterwmwong · Pull Request #1 · ljharb/test262

peterwmwong · 2018-03-19T00:59:50Z

Tests were updated and assume tc39/proposal-string-matchall#33 will be merged.

In an attempt to make it easier to see the coverage these tests provides, here's a version of the specification annotated with links to the related test from this PR: https://gist.github.com/peterwmwong/2a9d80d3533c0ab165bc8c675ac99e45 (Example below)

I haven't had the honor of contributing to Test262, so I'm not sure if I've made some bad cross cutting decisions...

const/let over var
Using arrow function
Frontmatter features, I'm adding String.prototype.matchAll and Symbol.matchAll

@ljharb I totally defer to you as you're the expert, let me know whatever I can do to make these tests better!

Thanks!

PS - If you'd like to try out my in-progress V8 implementation, checkout my branch or download the pre-built MacOS X binary here (zip): https://gist.github.com/peterwmwong/e38b50689d7717870b2cff7b68574b87

…matchall], and %RegExpStringIteratorPrototype% Tests were updated and assuming tc39/proposal-string-matchall#33 will be merged.

peterwmwong · 2018-03-19T01:00:22Z

test/built-ins/RegExp/prototype/Symbol.matchAll/get-species-constructor-err.js

@@ -1,22 +0,0 @@
-// Copyright (C) 2015 the V8 project authors. All rights reserved.


This test is now: species-constructor-get-species-throws.js

peterwmwong · 2018-03-19T01:00:50Z

test/built-ins/RegExp/prototype/Symbol.matchAll/this-val-non-obj.js

@@ -1,36 +0,0 @@
-// Copyright (C) 2018 Jordan Harband. All rights reserved.


This test is now: this-not-object-throws.js

Updated according to: Normative: when regexp arg is a string, minimize observable operations tc39/proposal-string-matchall#33

peterwmwong · 2018-03-19T01:02:32Z

test/built-ins/RegExp/prototype/Symbol.matchAll/this-val-non-regexp.js

@@ -1,22 +0,0 @@
-// Copyright (C) 2018 Jordan Harband. All rights reserved.


This test is now: isregexp-internal-regexp-is-false.js

Updated according to: Normative: when regexp arg is a string, minimize observable operations tc39/proposal-string-matchall#33

peterwmwong · 2018-03-19T01:02:54Z

test/built-ins/String/prototype/matchAll/cstm-matcher-get-err.js

@@ -1,26 +0,0 @@
-// Copyright (C) 2018 Jordan Harband. All rights reserved.


This test is now:

regexp-get-matchAll-throws

regexp-prototype-get-matchAll-throws

peterwmwong · 2018-03-19T01:03:12Z

test/built-ins/String/prototype/matchAll/invoke-builtin-matchall.js

@@ -1,43 +0,0 @@
-// Copyright (C) 2018 Jordan Harband. All rights reserved.


This test was removed as per changes from tc39/proposal-string-matchall#33

Given a string, @@matchall is no longer executed on the internally created
RegExp.

ljharb

Thanks, this is amazing!

Definitely please use var and normal functions (and no destructuring); these tests should work in a fully polyfilled ES5 implementation as much as they can.

ljharb · 2018-03-19T04:14:21Z

test/built-ins/RegExp/prototype/Symbol.matchAll/internal-regexp-lastindex-not-zero.js

+    RegExp.prototype[Symbol.matchAll].call({}, '');
+  });
+} finally {
+  Object.defineProperty(RegExp.prototype, Symbol.match, {


Tests generally don’t need to clean up after themselves; each test is run in its own environment.

Great! Removed: 7decf6a

ljharb · 2018-03-19T04:20:11Z

test/built-ins/RegExpStringIteratorPrototype/next/length.js

+
+assert.sameValue(RegExpStringIteratorProto.next.length, 0);
+
+verifyNotEnumerable(RegExpStringIteratorProto.next, "length");


We should try to use consistent quotes in this pr; probably single over double?

Absolutely, fixed: 0c1492d

ljharb · 2018-03-19T04:22:23Z

test/built-ins/RegExpStringIteratorPrototype/next/next-iteration.js

+const str = 'a*b';
+const iter = regexp[Symbol.matchAll](str);
+
+let { value, done } = iter.next();


We should probably make a helper for working with iterator so; maybe giving it the iterator and an array of expected values (or an array of functions to test each value), and have it test everything? Seems like it would DRY up a lot of these tests.

Yeah, sounds good to me. Is the following kinda what you had in mind?

// harness/compareIterator.js assert.compareIterator = function compareIterator(iter, validators) { var i = 0; var result = iter.next(); while (result.done !== false) { validators[i](result.value); result = iter.next(); i++; } } // harness/regExpUtils.js function matchComparator(expectedEntries, expectedIndex, expectedInput) { return function(match) { assert.compareArray(match, expectedEntries); assert.sameValue(match.index, expectedIndex); assert.sameValue(match.input, expectedInput); } } // Example usage assert.compareIterator(regexp[Symbol.matchAll](str), [ matchComparator(['a'], 0, str), matchComparator(['b'], 2, str) ]);

Yes, that looks great! Perhaps also with something that fails if the iterator is not "done" precisely after validators.length iterations?

Ah yes! I have implemented and applied the helpers (with your suggested "done" check) in the latest commit: 74aac5b

peterwmwong · 2018-03-20T13:46:29Z

@ljharb I believe I've addressed your initial round of review comments:

Remove cleanup code (undo prototype modifications)
Consistent quoting, use single quotes
ES5-ify: removed arrrow functions, let/const, and destructuring
Added assert.compareIterator and matchValidator

... and am ready for another round of review!
Thank you!

ljharb

Thank you!!

ljharb · 2018-03-20T14:21:01Z

harness/regExpUtils.js

+  return function(match) {
+    assert.compareArray(match, expectedEntries, 'Match entries');
+    assert.sameValue(match.index, expectedIndex, 'Match index');
+    assert.sameValue(match.input, expectedInput, 'Match input');


This should also take an expectedGroups argument, since match will either lack a groups property, or have one that's a null object when named capture group syntax is used.

I've rebased your branch and am going to merge it; so we can do that in a followup.

test

Fleshed out tests for Symbol.matchAll, String.p.matchAll, RegExp.p[@@…

ab1e265

…matchall], and %RegExpStringIteratorPrototype% Tests were updated and assuming tc39/proposal-string-matchall#33 will be merged.

peterwmwong requested a review from ljharb March 19, 2018 00:59

peterwmwong commented Mar 19, 2018

View reviewed changes

ljharb requested changes Mar 19, 2018

View reviewed changes

ljharb force-pushed the matchall-pww branch from 74aac5b to ab1e265 Compare March 20, 2018 23:33

ljharb approved these changes Mar 20, 2018

View reviewed changes

ljharb merged commit ab1e265 into ljharb:matchall Mar 20, 2018

ljharb mentioned this pull request Mar 20, 2018

Tests for String.prototype.matchAll tc39/test262#1500

Merged

ljharb pushed a commit that referenced this pull request Oct 3, 2019

Merge pull request #1 from tc39/master

5072e84

test

		@@ -1,22 +0,0 @@
		// Copyright (C) 2015 the V8 project authors. All rights reserved.

		@@ -1,36 +0,0 @@
		// Copyright (C) 2018 Jordan Harband. All rights reserved.

		@@ -1,26 +0,0 @@
		// Copyright (C) 2018 Jordan Harband. All rights reserved.

		@@ -1,43 +0,0 @@
		// Copyright (C) 2018 Jordan Harband. All rights reserved.


		assert.sameValue(RegExpStringIteratorProto.next.length, 0);

		verifyNotEnumerable(RegExpStringIteratorProto.next, "length");

Conversation

peterwmwong commented Mar 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ljharb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peterwmwong commented Mar 20, 2018

Uh oh!

ljharb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants