feat(test runner): make expect.extend immutable
#32366
Conversation
This comment has been minimized.
This comment has been minimized.
Skn0tt
changed the title
expect.extendImmutableexpect.extend immutable
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| @@ -61,6 +61,7 @@ import { | |||
| import { zones } from 'playwright-core/lib/utils'; | |||
| import { TestInfoImpl } from '../worker/testInfo'; | |||
| import { ExpectError, isExpectError } from './matcherHint'; | |||
| import { randomUUID } from 'node:crypto'; | |||
There was a problem hiding this comment.
We are using createGuid() in our source base - Playwright was originally targeting Node 12, so consider using it for consistency, it has the same power.
| if (typeof matcherName === 'string') { | ||
| for (const prefix of this._prefixes) { | ||
| for (let i = prefix.length; i > 0; i--) { | ||
| const qualifiedName = `${prefix.slice(0, i).join(':')}$${matcherName}`; |
There was a problem hiding this comment.
It would be nice to extract this key generation since it is used in two distant places.
| } | ||
|
|
||
| get(target: Object, matcherName: string | symbol, receiver: any): any { | ||
| let matcher = Reflect.get(target, matcherName, receiver); | ||
|
|
||
| if (typeof matcherName === 'string') { |
There was a problem hiding this comment.
It looks like moving this block below eliminates the check.
| @@ -104,11 +105,13 @@ export const printReceivedStringContainExpectedResult = ( | |||
|
|
|||
| type ExpectMessage = string | { message?: string }; | |||
|
|
|||
| function createMatchers(actual: unknown, info: ExpectMetaInfo): any { | |||
| return new Proxy(expectLibrary(actual), new ExpectMetaInfoProxyHandler(info)); | |||
| function createMatchers(actual: unknown, info: ExpectMetaInfo, prefix: string[], parentPrefixes: string[][]): any { | |||
There was a problem hiding this comment.
It took me a while to understand why parentPrefixes are needed. I'd suggest a variation of the implementation where instead of parentPrefixes each expect contains a growing list of matchers it has been extended to so far. In that case merge could be implemented like so:
export function mergeExpects(...expects: any[]) {
let current = expect;
for (const e of expects)
current = current.extend(e[getMatchersSymbol]);
return current;
}Would that work?
| function createExpect(info: ExpectMetaInfo) { | ||
| const getPrefixSymbol = Symbol('get prefix'); | ||
|
|
||
| function createExpect(info: ExpectMetaInfo, prefix: string[] = [], parentPrefixes: string[][] = []) { |
There was a problem hiding this comment.
In situations like this keep all the new params required, that would reduce the subsequent regressions where the param is omitted by accident.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Test results for "tests 1"1 failed 3 flaky30141 passed, 865 skipped Merge workflow run. |
|
I took a look at our docs for this. We're not showing the mutable case anywhere, so there's no changes needed. |
|
@pavelfeldman could you take another look? |
|
@Skn0tt this is indeed a breaking change. It causes custom asymmetric matchers to no longer get exposed on expect. With 1.47.1 the custom asymmetric matchers were accessible via I've created 2 PRs, one branched from latest main (#32740) and another one that branched right before this PR got merged (#32739) |
Changes
expect.extendbehaviour so that it doesn't mutate the global instance and behaves closer to what users expect. This is formally a breaking change, and I had to remove a test that asserts the breaking behaviour.TODO:
expect.extend