[regexp-named-groups] Expand tests for groups property
#1376
Conversation
There was a problem hiding this comment.
Tests look correct to me, so approved from my perspective.
It'd be nice to check that groups is a data property in the case of RegExps that don't have named capture groups. You can do this using the verifyProperty helper function used further down in the file.
In the review for #998, @leobalter encouraged me to break up the tests into several small files. Seems like you could keep following that here and put what you're adding into a new file, with this file dedicated to testing the new path and maintaining the existing file to test the other path.
| @@ -1,4 +1,5 @@ | |||
| // Copyright 2017 Aleksey Shvayka. All rights reserved. | |||
| // Copyright 2017 Mathias Bynens. All rights reserved. | |||
There was a problem hiding this comment.
FWIW other contributions from the V8 team came with "the V8 project authors" as the author, like the V8 source code.
mathiasbynens
force-pushed
the
named-groups-update
branch
from
84fe99d to
f405b39
Compare
|
Feedback addressed. Thanks for taking a look, @littledan! |
mathiasbynens
force-pushed
the
named-groups-update
branch
from
f405b39 to
278d7ee
Compare
| assert.sameValue("b", result.groups.a); | ||
| assert.sameValue("b", "ab".replace(re, "$<a>")); | ||
| assert.sameValue("c", "ab".replace(re, "$<b>")); | ||
| } |
There was a problem hiding this comment.
Each of these blocks could be a different file. This is not a blocker, and I will likely do it myself after I land this, but I figure I should give you the opportunity to make the split the way you want to. Let me know what you want to do.
There was a problem hiding this comment.
@rwaldron I’ll do the split — no worries! Naming things is hard, though… Gotta think of a good file name :)
| a. Let _groups_ be ObjectCreate(*null*). | ||
| 25. Else, | ||
| a. Let _groups_ be *undefined*. | ||
| 26. Perform ! CreateDataProperty(_A_, `"groups"`, _groups_). |
|
@mathiasbynens be sure you @ mention me when you reply to my comment—thanks! |
The `groups` property must be created unconditionally. tc39/proposal-regexp-named-groups#40
mathiasbynens
force-pushed
the
named-groups-update
branch
from
278d7ee to
74001fb
Compare
|
@rwaldron Split it up into separate files. PTAL |
|
@mathiasbynens you're the best, thank you. |
+1, thanks! :) |
|
Thanks to @schuay for writing these tests in the first place, as part of his work on implementing this change in V8! 💯 |
The
groupsproperty must be created unconditionally per tc39/proposal-regexp-named-groups#40.This patch ports @schuay’s tests from https://chromium.googlesource.com/v8/v8.git/+/d2f95e189c3209408525c88e2893d16c09724159.
cc @littledan