Add --skip-report-stdout and --skip-report-stderr options and extend attachment parsing #36
Conversation
Summary of ChangesHello @AndrianBdn, 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 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces valuable functionality by adding --skip-stdout and --skip-stderr options to reduce payload sizes, and by extending attachment parsing for JUnit XML files. The changes are well-implemented and include updates to documentation and tests. I have a couple of suggestions to improve code maintainability and test coverage.
| const testcases = await parsePlaywrightJson(jsonContent, '', { | ||
| skipStdout: 'never', | ||
| skipStderr: 'never', | ||
| }) |
There was a problem hiding this comment.
While the skip-stdout and skip-stderr options are passed here, this test file is missing specific tests to verify the functionality for Playwright JSON reports. It would be beneficial to add tests that cover cases like skipping output for passed tests and including it for failed tests. The tests in src/tests/junit-xml-parsing.spec.ts for the same feature can serve as a good reference.
| // Extract from failure elements (message attribute and text content) | ||
| for (const failure of tcase.failure || []) { | ||
| if (typeof failure === 'object') { | ||
| // Check message attribute | ||
| if (failure.$?.message) { | ||
| attachmentPaths.push(...extractAttachmentPaths(failure.$.message)) | ||
| } | ||
| // Check text content | ||
| if (failure._) { | ||
| attachmentPaths.push(...extractAttachmentPaths(failure._)) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Extract from error elements (message attribute and text content) | ||
| for (const error of tcase.error || []) { | ||
| if (typeof error === 'object') { | ||
| // Check message attribute | ||
| if (error.$?.message) { | ||
| attachmentPaths.push(...extractAttachmentPaths(error.$.message)) | ||
| } | ||
| // Check text content | ||
| if (error._) { | ||
| attachmentPaths.push(...extractAttachmentPaths(error._)) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Extract from skipped elements (message attribute and text content) | ||
| for (const skipped of tcase.skipped || []) { | ||
| if (typeof skipped === 'string') { | ||
| attachmentPaths.push(...extractAttachmentPaths(skipped)) | ||
| } else if (typeof skipped === 'object') { | ||
| // Check message attribute | ||
| if (skipped.$?.message) { | ||
| attachmentPaths.push(...extractAttachmentPaths(skipped.$.message)) | ||
| } | ||
| // Check text content | ||
| if (skipped._) { | ||
| attachmentPaths.push(...extractAttachmentPaths(skipped._)) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
There's significant code duplication in the logic for extracting attachment paths from failure, error, and skipped elements. This can be refactored into a helper function to improve readability and maintainability.
const extractAttachmentsFromElements = (
elements: (string | { _?: string; $?: { message?: string } })[] | undefined
) => {
for (const element of elements || []) {
if (typeof element === 'string') {
attachmentPaths.push(...extractAttachmentPaths(element));
} else if (typeof element === 'object') {
if (element.$?.message) {
attachmentPaths.push(...extractAttachmentPaths(element.$.message));
}
if (element._) {
attachmentPaths.push(...extractAttachmentPaths(element._));
}
}
}
};
// Extract attachments from failure, error, and skipped elements
extractAttachmentsFromElements(tcase.failure);
extractAttachmentsFromElements(tcase.error);
extractAttachmentsFromElements(tcase.skipped);
README.md
Outdated
| - `--attachments` - Try to detect and upload any attachments with the test result | ||
| - `--force` - Ignore API request errors, invalid test cases, or attachments | ||
| - `--ignore-unmatched` - Suppress individual unmatched test messages, show summary only | ||
| - `--skip-stdout` - Control when to skip stdout from test results (choices: `on-success`, `never`; default: `never`) |
There was a problem hiding this comment.
The changes looks okay, but the options look confusing to me (maybe its only me). My first impression was this flag is related to console output (stdout/stderr), but its related to the (stdout/stderr) data being uploaded for the test cases. Also its placed just after --ignore-unmatched which is about console output
Maybe rewording a bit might help ? Possibly "from uploaded test results" ?
There was a problem hiding this comment.
Thanks @hi-rai - very good point about the documentation!
…raction Adds two new command-line options for junit-upload and playwright-json-upload commands: - --skip-stdout: Control when to skip stdout from test results (on-success|never) - --skip-stderr: Control when to skip stderr from test results (on-success|never) Both options default to "never" (current behavior - always include output). When set to "on-success", stdout/stderr is excluded from passed tests but always included for failed, blocked, or skipped tests. This helps reduce payload size when tests have verbose logging while preserving debugging information for failures. Additionally, attachment extraction for JUnit XML has been extended to parse attachment markers from failure, error, and skipped element message attributes and text content, not just from system-out. This supports custom test framework configurations that embed attachment paths in error messages. Changes: - Added --skip-stdout and --skip-stderr command-line options to ResultUploadCommandModule - Updated Parser type signature to accept ParserOptions - Implemented conditional stdout/stderr filtering in junitXmlParser and playwrightJsonParser - Extended junitXmlParser to extract attachments from failure/error/skipped elements - Added comprehensive test coverage for new functionality - Updated README.md with usage examples Version bump: 0.4.1 → 0.4.2
…xtraction - Rename command-line flags from --skip-stdout/--skip-stderr to --skip-report-stdout/--skip-report-stderr to clarify these control stdout/stderr blocks from test reports, not qas-cli output - Update README documentation to reflect new flag names - Refactor duplicated attachment extraction logic into reusable helper function extractAttachmentsFromElements()
AndrianBdn
force-pushed
the
andrian/wdio-fixes-202511101404
branch
from
e2b4060 to
e183513
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces useful options (--skip-report-stdout and --skip-report-stderr) to conditionally exclude stdout and stderr from test reports, which can help reduce payload size for successful test runs. It also enhances JUnit XML parsing to extract attachments from failure, error, and skipped elements, improving support for frameworks like WebDriverIO. The changes are well-tested.
My review includes suggestions to:
- Shorten the new command-line option names for better usability.
- Improve type safety by explicitly typing function parameters.
- Prevent duplicate processing of attachments by using a
Set.
| 'skip-report-stdout': { | ||
| describe: 'When to skip stdout from test results', | ||
| type: 'string', | ||
| choices: ['on-success', 'never'] as const, | ||
| default: 'never' as const, | ||
| }, | ||
| 'skip-report-stderr': { | ||
| describe: 'When to skip stderr from test results', | ||
| type: 'string', | ||
| choices: ['on-success', 'never'] as const, | ||
| default: 'never' as const, | ||
| }, |
There was a problem hiding this comment.
The PR title and description use --skip-stdout and --skip-stderr, but the implementation uses the longer --skip-report-stdout and --skip-report-stderr. Shorter option names are generally better for command-line usability. I suggest renaming the options for consistency and brevity. This change would also need to be applied in README.md and src/utils/result-upload/ResultUploadCommandHandler.ts.
| 'skip-report-stdout': { | |
| describe: 'When to skip stdout from test results', | |
| type: 'string', | |
| choices: ['on-success', 'never'] as const, | |
| default: 'never' as const, | |
| }, | |
| 'skip-report-stderr': { | |
| describe: 'When to skip stderr from test results', | |
| type: 'string', | |
| choices: ['on-success', 'never'] as const, | |
| default: 'never' as const, | |
| }, | |
| 'skip-stdout': { | |
| describe: 'When to skip stdout from test results', | |
| type: 'string', | |
| choices: ['on-success', 'never'] as const, | |
| default: 'never' as const, | |
| }, | |
| 'skip-stderr': { | |
| describe: 'When to skip stderr from test results', | |
| type: 'string', | |
| choices: ['on-success', 'never'] as const, | |
| default: 'never' as const, | |
| }, |
| xmlString: string, | ||
| attachmentBaseDirectory: string | ||
| attachmentBaseDirectory: string, | ||
| options | ||
| ): Promise<TestCaseResult[]> => { |
There was a problem hiding this comment.
For better type safety and code clarity, it's a good practice to explicitly type the options parameter.
| xmlString: string, | |
| attachmentBaseDirectory: string | |
| attachmentBaseDirectory: string, | |
| options | |
| ): Promise<TestCaseResult[]> => { | |
| xmlString: string, | |
| attachmentBaseDirectory: string, | |
| options: ParserOptions | |
| ): Promise<TestCaseResult[]> => { |
| const attachmentPaths = [] | ||
|
|
||
| // Extract from system-out | ||
| for (const out of tcase['system-out'] || []) { | ||
| const text = typeof out === 'string' ? out : out._ ?? '' | ||
| if (text) { | ||
| attachmentPaths.push(...extractAttachmentPaths(text)) | ||
| } | ||
| } | ||
|
|
||
| // Helper function to extract attachments from failure/error/skipped elements | ||
| const extractAttachmentsFromElements = ( | ||
| elements: (string | { _?: string; $?: { message?: string } })[] | undefined | ||
| ) => { | ||
| for (const element of elements || []) { | ||
| if (typeof element === 'string') { | ||
| attachmentPaths.push(...extractAttachmentPaths(element)) | ||
| } else if (typeof element === 'object') { | ||
| if (element.$?.message) { | ||
| attachmentPaths.push(...extractAttachmentPaths(element.$.message)) | ||
| } | ||
| if (element._) { | ||
| attachmentPaths.push(...extractAttachmentPaths(element._)) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Extract attachments from failure, error, and skipped elements | ||
| extractAttachmentsFromElements(tcase.failure) | ||
| extractAttachmentsFromElements(tcase.error) | ||
| extractAttachmentsFromElements(tcase.skipped) | ||
|
|
||
| attachmentsPromises.push({ | ||
| index, | ||
| promise: getAttachments(attachmentPaths, attachmentBaseDirectory), |
There was a problem hiding this comment.
The current implementation collects attachment paths into an array. This could lead to duplicate paths if an attachment is referenced in multiple places (e.g., in both system-out and a failure message). Processing the same attachment multiple times is inefficient and might cause issues during upload. Using a Set to store attachment paths would elegantly solve this by automatically handling duplicates.
const attachmentPaths = new Set<string>()
// Extract from system-out
for (const out of tcase['system-out'] || []) {
const text = typeof out === 'string' ? out : out._ ?? ''
if (text) {
extractAttachmentPaths(text).forEach((path) => attachmentPaths.add(path))
}
}
// Helper function to extract attachments from failure/error/skipped elements
const extractAttachmentsFromElements = (
elements: (string | { _?: string; $?: { message?: string } })[] | undefined
) => {
for (const element of elements || []) {
if (typeof element === 'string') {
extractAttachmentPaths(element).forEach((path) => attachmentPaths.add(path))
} else if (typeof element === 'object') {
if (element.$?.message) {
extractAttachmentPaths(element.$.message).forEach((path) => attachmentPaths.add(path))
}
if (element._) {
extractAttachmentPaths(element._).forEach((path) => attachmentPaths.add(path))
}
}
}
}
// Extract attachments from failure, error, and skipped elements
extractAttachmentsFromElements(tcase.failure)
extractAttachmentsFromElements(tcase.error)
extractAttachmentsFromElements(tcase.skipped)
attachmentsPromises.push({
index,
promise: getAttachments(Array.from(attachmentPaths), attachmentBaseDirectory),| jsonString: string, | ||
| attachmentBaseDirectory: string | ||
| attachmentBaseDirectory: string, | ||
| options | ||
| ): Promise<TestCaseResult[]> => { |
There was a problem hiding this comment.
For better type safety and code clarity, it's a good practice to explicitly type the options parameter.
| jsonString: string, | |
| attachmentBaseDirectory: string | |
| attachmentBaseDirectory: string, | |
| options | |
| ): Promise<TestCaseResult[]> => { | |
| jsonString: string, | |
| attachmentBaseDirectory: string, | |
| options: ParserOptions | |
| ): Promise<TestCaseResult[]> => { |
- Use Set to deduplicate attachment paths in JUnit parser (prevents processing the same attachment multiple times when referenced in both system-out and failure/error messages) - Add explicit ParserOptions type annotation to both parsers for better type safety and code clarity
AndrianBdn
changed the title
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces two new command-line options, --skip-report-stdout and --skip-report-stderr, to control the inclusion of stdout and stderr from test results, which is particularly useful for reducing payload size with verbose logs. The changes also enhance attachment parsing for JUnit XML reports, allowing attachments to be extracted from failure, error, and skipped elements, not just system-out.
The implementation is well-structured, and the new functionality is covered by a comprehensive set of tests for both JUnit and Playwright report formats. The use of a Set to collect attachment paths is a nice touch to prevent duplicates.
My main suggestion is to consider renaming the new CLI options to be more concise (e.g., --skip-stdout), as mentioned in one of the review comments. This would improve the command-line user experience. There's also a minor inconsistency in the PR description where the shorter names are used, which this change would resolve.
Overall, this is a great enhancement to the tool.
| - `--skip-report-stdout` - Control when to skip stdout blocks from test report (choices: `on-success`, `never`; default: `never`) | ||
| - `--skip-report-stderr` - Control when to skip stderr blocks from test report (choices: `on-success`, `never`; default: `never`) |
There was a problem hiding this comment.
These option names are a bit long, and there's an inconsistency with the PR description which uses --skip-stdout and --skip-stderr. For better CLI usability and consistency, consider renaming --skip-report-stdout and --skip-report-stderr to the shorter --skip-stdout and --skip-stderr respectively. This change would need to be applied in src/commands/resultUpload.ts and src/utils/result-upload/ResultUploadCommandHandler.ts as well.
Add --skip-stdout and --skip-stderr options and extend attachment extraction
Adds two new command-line options for junit-upload and playwright-json-upload commands:
Both options default to "never" (current behavior - always include output).
When set to "on-success", stdout/stderr is excluded from passed tests but always included for failed, blocked, or skipped tests. This helps reduce payload size when tests have verbose logging while preserving debugging information for failures.
Additionally, attachment extraction for JUnit XML has been extended to parse attachment markers from failure, error, and skipped element message attributes and text content, not just from system-out. This supports custom test framework configurations that embed attachment paths in error messages.
Changes:
Version bump: 0.4.1 → 0.4.2