fix(selectivity): dont lose data on page navigate#1227
fix(selectivity): dont lose data on page navigate#1227KuznetsovRoman merged 3 commits intomasterfrom
Conversation
commit: |
4772fe7 to
1fd584d
Compare
| switch (domain) { | ||
| case "Target": | ||
| this.target.emit(method, cdpEventMessage.params); | ||
| this.target.emit(method, cdpEventMessage.params, cdpEventMessage.sessionId); |
There was a problem hiding this comment.
All events now also emit "sessionId" of events, if provided.
It helps to scope events and only handle those, which were emitted by our subscription
| await Promise.all([ | ||
| cdp.dom.enable(sessionId).then(() => cdp.css.enable(sessionId)), | ||
| cdp.target.setAutoAttach(sessionId, { autoAttach: true, waitForDebuggerOnStart: false }), | ||
| cdp.debugger.enable(sessionId), | ||
| cdp.page.enable(sessionId), | ||
| cdp.profiler.enable(sessionId), | ||
| ]); |
There was a problem hiding this comment.
Moved all domain enables here from "js-selectivity" and "css-selectivity" so we can wait them all at once
src/browser/cdp/selectivity/index.ts
Outdated
| if (callFrames[0]?.functionName !== testplaneCoverageBreakScriptName) { | ||
| cdp.debugger.resume(sessionId).catch(() => {}); | ||
| return; | ||
| } |
There was a problem hiding this comment.
In case its not our breakpoint, just resume the execution
| } | ||
| }); | ||
|
|
||
| let pageSwitchPromise: Promise<void> = Promise.resolve(); |
There was a problem hiding this comment.
As CDP might clean script data when switching pages, we solve the issue like this:
Call for "debugger" on page "beforeunload" event
It stops browser's V8
While V8 is stopped, take incremental snapshot of JS and CSS coverage
Wait for selectivity assets to load (they will be unable to download on page swtich)
Resume V8
| return; | ||
| } | ||
|
|
||
| pageSwitchPromise = pageSwitchPromise.finally(() => |
There was a problem hiding this comment.
Saved to promise so we can await it before dependencies recording stops
| const [cssDependencies, jsDependencies] = await Promise.all([ | ||
| await pageSwitchPromise; | ||
|
|
||
| const [cssDependenciesPromise, jsDependenciesPromise] = await Promise.allSettled([ |
There was a problem hiding this comment.
Switched to "allSettled" because its important to unsubscribe from "debugger paused"
pageSwitchPromise can't throw either
| return this._cdp.debugger.resume(this._sessionId).catch(() => {}); | ||
| }); | ||
| // If we haven't got "scriptParsed" event for the script, pull up source code + source map manually | ||
| private _ensureScriptsAreLoading(coverage: CDPScriptCoverage[]): void { |
There was a problem hiding this comment.
Moved original L137 - L207 to separate method. No changes
| Promise.allSettled(Object.values(this._scriptsSource)), | ||
| Promise.allSettled(Object.values(this._scriptsSourceMap)), |
There was a problem hiding this comment.
Errors will be handled on "stop" call
| } | ||
|
|
||
| // If we haven't got "styleSheetAdded" event for the script, pull up styles + source map manually | ||
| private _ensureStylesAreLoading(ruleUsage: CSSRuleUsage[]): void { |
There was a problem hiding this comment.
Moved original L106 - L155 to separate method. No changes
| } | ||
|
|
||
| private async _waitForLoadingStyles(): Promise<void> { | ||
| await Promise.allSettled(Object.values(this._stylesSourceMap)); |
There was a problem hiding this comment.
Errors are handled on "stop" call
| { header: { styleSheetId, sourceURL, sourceMapURL } }: CssEvents["styleSheetAdded"], | ||
| cdpSessionId?: CDPSessionId, | ||
| ): void { | ||
| if (!this._sessionId || cdpSessionId !== this._sessionId) { |
There was a problem hiding this comment.
Avoid handling styles for different sessions
Just in case user is subscribed to "css" in another target
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1fd584da12
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| cdp.debugger.on("paused", debuggerPausedFn); | ||
|
|
||
| await cdp.page.addScriptToEvaluateOnNewDocument(sessionId, { source: scriptToEvaluateOnNewDocument }); |
There was a problem hiding this comment.
Remove injected beforeunload script during selectivity teardown
startSelectivity adds a new Page.addScriptToEvaluateOnNewDocument handler on every test run, but teardown never calls Page.removeScriptToEvaluateOnNewDocument. Because this API stores scripts in a persistent list for the target, long-lived browser sessions will accumulate duplicate beforeunload listeners, causing multiple debugger pauses/snapshots per navigation and progressively slower or flaky runs.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
We dont have to do it because session is detached in returned stop function like this: cdp.target.detachFromTarget(sessionId).catch(() => {});
For other new tests we would have new cdp target id
| this._ensureScriptsAreLoading(coveragePart.result); | ||
|
|
||
| return sourceMap; | ||
| } catch (err) { | ||
| return err as Error; | ||
| } | ||
| }); | ||
| }); | ||
| await this._waitForLoadingScripts(); |
There was a problem hiding this comment.
Avoid blocking paused navigation on source-map loading
takeCoverageSnapshot now waits for _waitForLoadingScripts() to settle before returning, and this function is called from the Debugger.paused handler before Debugger.resume. In pages with slow/unreachable source-map URLs, navigation stays paused until those fetches settle, which can turn ordinary page transitions into command timeouts.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Thats the point: switching page erases those scripts from v8
1fd584d to
204f0e4
Compare
No description provided.