Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork742
fix : naming convention session / main erasing with long test.title/tags#4992
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:3.x
Are you sure you want to change the base?
fix : naming convention session / main erasing with long test.title/tags#4992
Uh oh!
There was an error while loading.Please reload this page.
Conversation
fix: context is funtion in session and not propertyfix : dual saving main session
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
[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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
[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 |
Run our tests and still see the error, maybe this needs deeper investigation.
|
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)