fix : naming convention session / main erasing with long test.title/tags #4992
Conversation
fix: context is funtion in session and not property fix : dual saving main session
There was a problem hiding this comment.
Pull Request Overview
This PR fixes issues related to saving video and trace artifacts in multi-session Playwright tests by correcting the naming convention and the use of context as a function.
- Renames artifact files by prepending the session name to the test title.
- Fixes the context access by calling it as a function.
- Excludes the main session (represented by an empty string) from the sessions loop to prevent duplicate saving.
| @@ -2377,15 +2377,16 @@ class Playwright extends Helper { | |||
| if (this.options.recordVideo && this.page && this.page.video()) { | |||
| test.artifacts.video = saveVideoForPage(this.page, `${test.title}.failed`) | |||
| for (const sessionName in this.sessionPages) { | |||
| test.artifacts[`video_${sessionName}`] = saveVideoForPage(this.sessionPages[sessionName], `${test.title}_${sessionName}.failed`) | |||
| if (sessionName === '') continue | |||
There was a problem hiding this comment.
[nitpick] Consider defining a constant (e.g. MAIN_SESSION) for the main session check instead of using an empty string literal, to enhance code clarity and maintainability.
| if (sessionName === '') continue | |
| if (sessionName === MAIN_SESSION) continue |
Copilot uses AI. Check for mistakes.
| } | ||
| } | ||
|
|
||
| if (this.options.trace) { | ||
| test.artifacts.trace = await saveTraceForContext(this.browserContext, `${test.title}.failed`) | ||
| for (const sessionName in this.sessionPages) { | ||
| if (!this.sessionPages[sessionName].context) continue | ||
| test.artifacts[`trace_${sessionName}`] = await saveTraceForContext(this.sessionPages[sessionName].context, `${test.title}_${sessionName}.failed`) | ||
| if (!this.sessionPages[sessionName].context || sessionName === '') continue |
There was a problem hiding this comment.
[nitpick] Similarly, use a named constant for the main session check in the trace saving logic to improve clarity and avoid hard-coding empty string comparisons.
| if (!this.sessionPages[sessionName].context || sessionName === '') continue | |
| if (!this.sessionPages[sessionName].context || sessionName === MAIN_SESSION_NAME) continue |
Copilot uses AI. Check for mistakes.
| @@ -2399,7 +2400,8 @@ class Playwright extends Helper { | |||
| if (this.options.keepVideoForPassedTests) { | |||
| test.artifacts.video = saveVideoForPage(this.page, `${test.title}.passed`) | |||
| for (const sessionName of Object.keys(this.sessionPages)) { | |||
| test.artifacts[`video_${sessionName}`] = saveVideoForPage(this.sessionPages[sessionName], `${test.title}_${sessionName}.passed`) | |||
| if (sessionName === '') continue | |||
There was a problem hiding this comment.
[nitpick] Consider using a consistent iteration approach (for example, either for...in or Object.keys) across the different loops for handling sessions to improve readability.
Copilot uses AI. Check for mistakes.
|
@julien-ft-64 may you suggest if we could add some tests for those changes? https://github.com/codeceptjs/CodeceptJS/blob/3.x/test/helper/Playwright_test.js#L1431 |
Motivation/Description of the PR
Solve issue in some use case where videos and traces of Playwright are not saved correctly when using multiple sessions
fix : naming convention session / main erasing with long test.title/tags
--> file name of traces and video have title limited to 245 char and contains sessions name at end and test status, which in case of gherkin/bdd is not enough, the session name is truncated. Meaning all traces/videos of sessions are written to same file. Changed the naming to have the session before test title, therefore, having always a different file name
fix: context is funtion in session and not property
--> in the sessions iteration, the access to context is not a property but a function, it resulted in getting out of saving because no tracing property existed. Added the () to context usage for sessions loop
fix : dual saving main session
--> main session is also in the list of sessions, generating a Playwright error as the traces are already savec for the main session, and having 2 times the session saved. Added a check on session name to exclude it from the sessions loop.
Applicable helpers:
Applicable plugins:
Type of change
Checklist:
npm run docs)npm run lint)npm test)