rename(std/testing): rename assert*Contains to assert*Includes

[timreichen]

#6888

Rename functions in asserts.ts

• assertStringContains -> assertStringIncludes
• assertArrayContains -> assertArrayIncludes

[bartlomieju]

I'm generally in favor of this change, the only point of discussion is original suggestion from #6888 if we should have both assertStringIncludes and assertArrayIncludes or just assertIncludes as implemented in this PR. Given that assertEquals is magical and works on all values makes assertIncludes favorable.

CC @kitsonk @nayeemrmn @caspervonb

[caspervonb]

Difficult to see what has actually changed here with 232 changed files in the diff but in favour of generalising on a.includes(b) as long as we provide rich pretty printed diffs for both strings and array-likes (otherwise, if we don't provide a better diff assert(a.includes(b)) is just as good)

[nayeemrmn]

if we should have both assertStringIncludes and assertArrayIncludes or just assertIncludes as implemented in this PR

I think they should definitely be different functions. They have different and unrelated behaviour, which would be especially confusing since strings are iterable. The JS methods are namespaced to Array.prototype.includes() and String.prototype.includes(). Making them overloads is illogical and makes both the implementation and call sites harder to read.

[timreichen]

@caspervonb The special casing is because of the overload. To avoid that, @nayeemrmn suggestion might be the better solution. Just a rename of contains to includes.

[bartlomieju]

if we should have both assertStringIncludes and assertArrayIncludes or just assertIncludes as implemented in this PR

I think they should definitely be different functions. They have different and unrelated behaviour, which would be especially confusing since strings are iterable. The JS methods are namespaced to Array.prototype.includes() and String.prototype.includes(). Making them overloads is illogical and makes both the implementation and call sites harder to read.

Thanks for being voice of reason @nayeemrmn. In this light I agree that we should just do the rename without merging those assertion. @timreichen could you please update the PR?

[timreichen]

@bartlomieju already did:)

[bartlomieju]

@timreichen this PR shouldn't change 3k lines diff - I think something went wrong during rebase, please check

[timreichen]

@bartlomieju Shoot, I suck at rebasing. Sorry. Done now.

[bartlomieju]

LGTM and I'm in favor of landing this PR

@lucacasonato @kitsonk please take a look

[lucacasonato]

LGTM. We should definitely mention this in the blog post.