-
Notifications
You must be signed in to change notification settings - Fork 0
Support Playwright JSON #33
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Summary of ChangesHello @hi-rai, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the QAS CLI by introducing full support for uploading Playwright JSON test reports. This new capability is integrated through a refactored and generalized command structure, which now handles both JUnit XML and Playwright JSON reports using a common logic base. The update ensures that test results from Playwright can be seamlessly submitted to QA Sphere, complete with test case matching and attachment handling. The accompanying documentation has been revised to reflect these changes, providing clear guidance for users. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
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.
Code Review
This pull request introduces support for Playwright JSON reports, which is a great addition. The refactoring to abstract the parsing and uploading logic is well-executed, creating a clean and extensible architecture. The introduction of a standardized TestCaseResult and a generic ResultUploadCommandHandler is a solid design choice. The new test suites, especially the parameterized tests covering both JUnit and Playwright formats, are thorough and ensure the stability of the refactored code.
I've found a potential bug in the result comment construction and a couple of minor maintainability improvements. Overall, this is a high-quality contribution.
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.
Same as older junit-upload.ts, but now support both junit xml and playwright json
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.
Instead of deleting and copying code, renaming the file will result in better diffs.
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.
It doesn't matter if we rename a file and modify it or delete and create a new similar one, git handles both cases in the same fashion on its own.
Git detects renames based on file similarity - if changes are too much, it would consider it as delete+create rather than rename+modify
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.
The playwright json files are similar to junit xmls with same name. I didn't create it manually, but was generated by AI
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.
Changed from spaces to tabs and the status texts are now as per QA Sphere
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.
Same as older junit-upload.spec.ts, but now support both junit xml and playwright json commands
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.
Similar to older JUnitResultUploader.ts, but now supports both playwright json and junit xml
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.
Space to tabs
src/utils/projectExtractor.ts
Outdated
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.
Is now simpler and moved to ResultUploadCommandHandler
satvik007
left a comment
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.
Thanks @hi-rai
The PR looks pretty good to me, but I spent a lot of time understanding it and wrote some optional comments.
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.
Instead of deleting and copying code, renaming the file will result in better diffs.
| name: markerFromAnnotations | ||
| ? `${markerFromAnnotations}: ${titlePrefix}${spec.title}` | ||
| : `${titlePrefix}${spec.title}`, |
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.
Instead of writing the annotation to the name and then again using a regex to get the tcase seq from name. Can we create a field for seq in the parsing stage itself.
The current logic is sound but I think it will be better to extract seq while parsing itself.
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.
Had done that initially, but changed it as Prefixing it to name also helps in detectProjectCode
| for (let i = 0; i < results.length; i++) { | ||
| const { tcase, result } = results[i] | ||
| let comment = result.message | ||
| loader.setText(`Uploading test case ${i + 1} of ${results.length}`) | ||
| if (this.args.attachments) { | ||
| const attachmentUrls: Array<{ name: string; url: string }> = [] | ||
| for (const attachment of result.attachments) { | ||
| if (attachment.buffer) { | ||
| const { url } = await this.api.file.uploadFile( | ||
| new Blob([attachment.buffer]), | ||
| attachment.filename | ||
| ) | ||
| attachmentUrls.push({ url, name: attachment.filename }) | ||
| } | ||
| } | ||
| comment += `\n<h4>Attachments:</h4>\n${makeListHtml(attachmentUrls)}` | ||
| } | ||
|
|
||
| await this.api.runs.createResultStatus(this.project, this.run, tcase.id, { | ||
| status: result.status, | ||
| comment, | ||
| }) | ||
| } |
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.
We should think of some performance improvements with parallel uploads and api calls.
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.
Better to do it separately. I will open another PR after this is merged
| if (conflictMatch) { | ||
| const existingRunId = Number(conflictMatch[1]) | ||
| console.log(chalk.yellow(`Reusing existing test run "${title}" with ID: ${existingRunId}`)) | ||
| return existingRunId | ||
| } |
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.
Another common error could be that we couldn't match the seq ids while creating test runs, can we handle that as well.
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.
That won't happen as getTestCases only returns found tcases, which we are using here.
Currently we are silently skipping if some tcases are not found getTestCases, as long as at least one tcase is found - not sure if that is intentional or something that was missed. I haven't changed the behavior, but please let me know if we need to do that
AndrianBdn
left a comment
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.
That looks awesome, @hi-rai - thanks! We are going to publish the new version right away and even mention that we support two logging formats in the marketing announcements.
This fixes https://github.com/Hypersequent/tms-issues/issues/1998