Conversation
This is a convenience function which tries to make tests easier to read and write.
Tests for the stage 3 proposal at https://tc39.github.io/proposal-regexp-lookbehind/ Tests ported from V8, written by @hashseed
leobalter
left a comment
leobalter
left a comment
There was a problem hiding this comment.
I suggested some split on these tests, as with that, we might also structure the files to be all prefixed with lookbehind- including the lookbehind-negative- parts.
Something that would be interesting is finding negative cases where the lookbehind pattern is malformed.
Other part that I'm missing is checking the tests with match only. Adding some simple test, exec and replace would be interesting as well.
|
|
||
| message += 'Expected SameValue(«' + String(actual) + '», «' + String(expected) + '») to be true'; | ||
|
|
||
| $ERROR(`${message}${message === undefined ? '' : ' '}Expected the arrays [${actual}] to have the same contents as [${expected}]`); |
There was a problem hiding this comment.
message is never undefined here.
Also, it seems the message is redundant. Line 26 already cast actual and expected to String values.
My suggestion for this function:
assert.compareArray = function(actual, expected, message = '') {
assert(
compareArray(actual, expected),
`Expected ${String(actual)} and ${String(expected)} to have the same contents. ${message}`
);
}There was a problem hiding this comment.
Gah, I meant to delete those lines in the middle!
Changed as suggested.
| assert.compareArray(["def"], "abcdefdef".match(/(?<=^\w+)def/)); | ||
| assert.compareArray(["def", "def"], "abcdefdef".match(/(?<=^\w+)def/g)); | ||
|
|
||
| // Word boundary matches. |
There was a problem hiding this comment.
splitting these tests in multiple files help identifying the purpose of each one.
Plus, it helps identifying multiple aspects.
There was a problem hiding this comment.
Would splitting up the tests help more than the comments in the test file helps? The tests in a particular file are all getting at the same bit of spec text.
| assert.compareArray(["abc", "abc"], /(abc\1)/.exec("abc\u1234")); | ||
| assert.compareArray(["abc", "abc"], /(abc\1)/i.exec("abc")); | ||
| assert.compareArray(["abc", "abc"], /(abc\1)/i.exec("abc\u1234")); | ||
| var oob_subject = "abcdefghijklmnabcdefghijklmn".substr(14); |
There was a problem hiding this comment.
why not use "abcdefghijklmn"?
There was a problem hiding this comment.
Many JS implementations use a sort of virtual slice for situations like this, without copying. This test verifies that such implementations don't make the mistake of allowing lookbehind to go beyond the start of the string.
There was a problem hiding this comment.
If the idea is catching very specific behaviors, that's why I insist we should have split files.
At least a comment explaining this, otherwise the substr seems completely unnecessary for the purpose of testing the final value in oob_subject
There was a problem hiding this comment.
@littledan ...
Many JS implementations use a sort of virtual slice for situations like this, without copying. This test verifies that such implementations don't make the mistake of allowing lookbehind to go beyond the start of the string.
With respect, I have a hard time believing that the code you're referring to is actually doing what you claim it's doing.
There was a problem hiding this comment.
Since this test originates from V8, this targets V8's sliced strings, which are used for substrings above a length of 13 characters. I wrote this test for exactly the reason @littledan mentioned. That's why the variable name is called oob_subject. The underlying string backing store extends beyond the actual boundary of the sliced string.
There was a problem hiding this comment.
I only need some sort of description like a comment - it's better in a separate file - otherwise this can be removed in any further maintenance because it's implementation specific and I can't observe the behavior from the specs.
There was a problem hiding this comment.
Ok, so it's a purely V8 implementation specific issue? If that's the case, and it's not testing anything that's actually observable per spec, then it falls in one of the following:
- Doesn't belong here.
- Belongs in a special "shame" file.
There was a problem hiding this comment.
Honestly, this is like saying that test262 should start supporting V8's testing macros.
There was a problem hiding this comment.
I agree that that part should be removed.
I don't feel really strongly about this, and I think neither does @littledan. He simply adapted the tests I wrote while implementing lookbehind for V8 because that is more convenient than coming up with ones from scratch. It's not like we are trying to force them into test262.
There was a problem hiding this comment.
I disagree that we should have such a high bar for getting test262 tests in, that we shouldn't have tests that target possible bug sources. If it is a test that's correct according to the spec, is it really creating such a big maintenance burden? I bet V8 isn't the only engine to treat strings like this either--this is likely useful for other engines too. Having such strict requirements for tests just discourages contributions and gives us all less test coverage.
|
lookBehing is in stage 3, so this PR has blocker other than the review feedback. |
|
I'll get this fixed |
4 participants
Thanks @hashseed for writing these tests; this patch is a simple port.