Add tests for String#replaceAll

[leobalter]

Closes #2417

[leobalter]

cc @mathiasbynens @ljharb @rwaldron

[mathiasbynens]

Thanks for working on this!

I was hoping to see:

• tests for GetSubstitution
• tests with the sticky flag (verifying “the sticky gotcha”)

Maybe I missed them?

[leobalter]

@mathiasbynens I'll be looking onto these today.

[leobalter]

@mathiasbynens we have better tests for GetSubstitution now.

Would you give me an example on the sticky flags?

The problem I'm finding here is that if a SearchValue is a RegExp, it will call the Symbol.replace method from it, and ignore the remainder of the replaceAll parts.

If we cut off the @@replace from the RE, it will get the toString value, so it won't make much difference.

[syg]

Would you give me an example on the sticky flags?

I believe the "sticky gotcha" that @mathiasbynens is referring to is tc39/proposal-string-replaceall#17 (comment): a sticky global regexp will end up replacing all consecutive matches from the beginning of the string, otherwise nothing at all.

+1 would definitely be good to see this test.

[mathiasbynens]

Confirming what @syg said. I was indeed referring to https://docs.google.com/presentation/d/1OGmV6uVTOEeSYO1nMeLjzflkbRJZ4p9QXlGV8IvDMmU/edit#slide=id.g61fdbd908b_0_0. The V8 tests for replaceAll could be an interesting reference, too.

[mathiasbynens]

Looking at the GetSubstitution commit, there’s very few tests for GetSubstitution + regular expressions (i.e. the main use case of GetSubstitution). Could we add those? I imagine there might already be such tests for replace that we can reuse.

[leobalter]

WRT sticky flags:

There isn't any gotcha for replaceAll and regexp sticky flags, if it's given a RegExp-like object as the searchValue, it will call this object's @@replace. The gotcha is from there. We have coverage it's called, e.g. here and here using an actual RegExp, verifying all the parameters, this value, and return. The gotcha parts should be covered in RegExp.prototype[Symbol.replace], which I'll check if we have tests for.

We also have coverage for a given RegExp searchValue we know that replaceAll won't even call ToString for the this value, searchValue and replaceValue. This one needs a custom searchValue object for full observation.

I can add a test that observes a result from RegExp.prototype[Symbol.replace] inside String#replaceAll given a RegExp searchValue, but I honestly think this is not useful if we have proper coverage for each feature.

---

WRT GetSubstitution

Looking at the GetSubstitution commit, there’s very few tests for GetSubstitution + regular expressions (i.e. the main use case of GetSubstitution). Could we add those? I imagine there might already be such tests for replace that we can reuse.

We have other tests with other prefixes - and not from this commit - where we cover regular expressions. I tried to add samples using different string length sizes of the this value, searchValue, and replaceValue.

These are much more related to StringIndexOf and finding matching positions. They also include cases like the last position in the string even for an empty string on any value.

The tests prefixed with GetSubstitution are really capturing the very special cases of Table 53.

I can assure you we already have a much better coverage than String#replace, it's just a nightmare organizing everything we have in the legacy tests part.

Can you give me a more specific idea on what I am missing in these tests?

[mathiasbynens]

There isn't any gotcha for replaceAll and regexp sticky flags, if it's given a RegExp-like object as the searchValue, it will call this object's @@replace. The gotcha is from there.

The gotcha is developer-facing: it’s behavior that could be considered as surprising (both by JS devs as by engine implementers). That’s why we need explicit test coverage for it: we must make sure that replaceAll behaves correctly even in this unusual case.

The higher-level theme here is that we should ensure (i.e. codify in tests) that replaceAll and replace have the same behavior for the cases where the spec says they should.

Can you give me a more specific idea on what I am missing in these tests?

Tests for the combination of regular expressions + GetSubstitution (which is the main use case of GetSubstitution). Things like 'abcba'.replaceAll(/b/g, "$'") and 'abcabcabcabc'.replaceAll(/a(b)c/g, '$1') — for each special substitution case. Again, replaceAll doesn’t offer any new behavior here, but we should ensure that it matches replace (and so probably test both replace and replaceAll in these tests).

I can assure you we already have a much better coverage than String#replace, it's just a nightmare organizing everything we have in the legacy tests part.

That’s great! Most of the new replaceAll tests should probably test replace as well (since in most cases the expected result is the same, and like you said replace coverage might be lacking). The only tests where replaceAll behaves differently should be the non-global RegExp case and the case where searchValue is a string.

[leobalter]

Tests for the combination of regular expressions + GetSubstitution (which is the main use case of GetSubstitution). Things like 'abcba'.replaceAll(/b/g, "$'") and 'abcabcabcabc'.replaceAll(/a(b)c/g, '$1')

If we have multiple tests verifying replaceAll will stop and return anything from RegExp#@@replace, it's very hard for me to see an improvement in the coverage. If such, it might be interesting for this test to also include a similar case.

I'll add GetSubstitution cases for the test file where we check a call to RegExp#@@replace and it's returned value.

[leobalter]

I'd prefer to add tests for String#replace (and potentially RegExp#@@replace) in a separate PR. We can work towards merging this one first.

[leobalter]

@mathiasbynens PTAL at the new changes.

[mathiasbynens]

The additional tests look good. Can we get some tests for replacer being a function going?

[leobalter]

@mathiasbynens PTAL

[mathiasbynens]

LGTM, let’s land this! 🎉

[rkirsling]

Wow, this test is intense—even with replace itself, neither JSC nor SM pass it. I guess that's fine though, if the replace version is coming in a future PR.

[leobalter]

I believe this is much more important to RegExp.prototype[Symbol.replace], which is the part we are really testing here. So my plan is in this priority order:

1. Extend tests coverage for RegExp.prototype[Symbol.replace]
2. Create a feature flag for RegExp.prototype[Symbol.replace] and String.prototype.replace. Not new features, but there are too much tangliness here. Yes, I invented a new word.
3. Extend tests coverage for String.prototype.replace
4. Remove redundant legacy tests for String#replace we will eventually find as duplicates of the new coverage from the previous item.

That won't hurt anyone and will only add coverage. If the tests are failing on 2 engines, I believe this points to a bug withing the RegExp method, which we are missing to flag in the appropriate place.