Conversation
Add a new `allure-upload` CLI command that parses Allure results directories and uploads test results to QA Sphere. Unlike the existing file-based parsers, this handles directory input containing `*-result.json` files. Key features: - Zod-validated parsing of Allure result JSON files - Status mapping (broken→blocked, unknown→passed) - TMS link marker extraction (URL parsing and name regex) - Folder derivation from suite/parentSuite/feature labels - Attachment support via source file references - Parameterized test name display - Graceful skipping of malformed/invalid files and containers Also refactors integration tests to use per-format expected counts instead of hardcoded values, making it easier to add formats with different fixture sizes. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Claude finished @satvik007's task in 4m 53s —— View job Review complete
Submitted a review with 6 inline comments covering 1 high, 1 medium, 2 low, and 2 nit-level findings. The most notable is a path traversal risk via untrusted |
Summary of ChangesHello @satvik007, 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 introduces the Highlights
Changelog
Activity
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 support for uploading Allure test results. The changes are well-structured, adding a new allure-upload command, a robust Allure parser with Zod validation, and comprehensive unit tests. The integration tests have also been refactored to be more maintainable and now include Allure, which is a great improvement. I've suggested a few minor refactorings in the new Allure parser for improved readability and a change in the unit tests to enhance portability. Overall, this is a solid contribution.
| }) | ||
|
|
||
| test('Should map unknown status to passed', async () => { | ||
| const tmpDir = join('/tmp', 'allure-unknown-test') |
There was a problem hiding this comment.
Using a hardcoded /tmp path for temporary directories is not platform-independent and may cause issues on operating systems like Windows. It's better to use os.tmpdir() to get the system's temporary directory path.
You'll need to add import { tmpdir } from 'node:os'; at the top of the file. Please apply this change to all temporary directory creations in this test suite.
| const tmpDir = join('/tmp', 'allure-unknown-test') | |
| const tmpDir = join(tmpdir(), 'allure-unknown-test') |
| const getFolderFromLabels = (labels: AllureResult['labels']): string => { | ||
| if (!labels || labels.length === 0) return '' | ||
|
|
||
| const labelMap = new Map<string, string>() | ||
| for (const label of labels) { | ||
| if (!labelMap.has(label.name)) { | ||
| labelMap.set(label.name, label.value) | ||
| } | ||
| } | ||
|
|
||
| return ( | ||
| labelMap.get('suite') || | ||
| labelMap.get('parentSuite') || | ||
| labelMap.get('feature') || | ||
| labelMap.get('package') || | ||
| '' | ||
| ) | ||
| } |
There was a problem hiding this comment.
This function can be made more concise by using Array.prototype.find() directly instead of first building a Map. This avoids the intermediate data structure and makes the intent clearer.
const getFolderFromLabels = (labels: AllureResult['labels']): string => {
if (!labels || labels.length === 0) return ''
const findLabelValue = (name: string) => labels.find((l) => l.name === name)?.value
return (
findLabelValue('suite') ||
findLabelValue('parentSuite') ||
findLabelValue('feature') ||
findLabelValue('package') ||
''
)
}| const buildMessage = ( | ||
| result: AllureResult, | ||
| status: ResultStatus, | ||
| options: ParserOptions | ||
| ): string => { | ||
| let message = '' | ||
| const details = result.statusDetails | ||
|
|
||
| if (!details) return message | ||
|
|
||
| const includeMessage = !(status === 'passed' && options.skipStdout === 'on-success') | ||
| const includeTrace = !(status === 'passed' && options.skipStderr === 'on-success') | ||
|
|
||
| if (includeMessage && details.message) { | ||
| message += `<h4>Message:</h4><pre><code>${escapeHtml(details.message)}</code></pre>` | ||
| } | ||
|
|
||
| if (includeTrace && details.trace) { | ||
| message += `<h4>Trace:</h4><pre><code>${escapeHtml(details.trace)}</code></pre>` | ||
| } | ||
|
|
||
| return message | ||
| } |
There was a problem hiding this comment.
This function can be refactored for better readability by using an array to collect message parts and then joining them at the end. This avoids string concatenation with += and makes the conditions for including each part a bit clearer.
const buildMessage = (
result: AllureResult,
status: ResultStatus,
options: ParserOptions
): string => {
const details = result.statusDetails
if (!details) {
return ''
}
const messages: string[] = []
const isPassed = status === 'passed'
if (details.message && !(isPassed && options.skipStdout === 'on-success')) {
messages.push(`<h4>Message:</h4><pre><code>${escapeHtml(details.message)}</code></pre>`)
}
if (details.trace && !(isPassed && options.skipStderr === 'on-success')) {
messages.push(`<h4>Trace:</h4><pre><code>${escapeHtml(details.trace)}</code></pre>`)
}
return messages.join('')
}| const attachmentPaths: string[] = [] | ||
| if (result.attachments) { | ||
| for (const attachment of result.attachments) { | ||
| attachmentPaths.push(attachment.source) | ||
| } | ||
| } |
There was a problem hiding this comment.
This block of code for collecting attachment paths can be simplified into a single line using optional chaining and Array.prototype.map().
const attachmentPaths = result.attachments?.map((a) => a.source) ?? []![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
Well-structured PR that adds the allure-upload command following existing patterns. The Allure parser is clean, the Zod schemas are thorough, and the integration test refactor to use per-format expected counts is a good improvement. The test coverage is solid with 28 unit tests and 20 integration tests.
Key findings:
| Severity | Issue | Location |
|---|---|---|
| High | Attachment source paths from untrusted JSON are not validated for path traversal | allureParser.ts:220-226 |
| Medium | readdirSync on non-existent directory throws raw ENOENT (no user-friendly error) |
allureParser.ts:169 |
| Low | TMS link name regex is broader than the pattern used elsewhere in the codebase | allureParser.ts:120 |
| Low | No guard against negative duration from corrupted timestamps | allureParser.ts:205 |
| Nit | z.any() should be z.unknown() for type safety |
allureParser.ts:67 |
| Nit | Weak assertion (toBeGreaterThanOrEqual) should be exact count |
allure-parsing.spec.ts:19 |
Not flagged (no issues found): Performance — the parser is efficient with O(n) complexity, correct use of Promise.all for attachment resolution, and appropriate sync I/O for a CLI tool. No performance concerns.
| // Collect attachment file paths | ||
| const attachmentPaths: string[] = [] | ||
| if (result.attachments) { | ||
| for (const attachment of result.attachments) { | ||
| attachmentPaths.push(attachment.source) | ||
| } | ||
| } |
There was a problem hiding this comment.
Path traversal via untrusted attachment source paths (High)
The attachment.source values are read from untrusted Allure JSON files and passed directly to getAttachments(), which calls path.join(basePath, filePath). A malicious or tampered result file could set source to ../../etc/passwd or similar, causing arbitrary file reads. The contents would then be uploaded to the QA Sphere API.
This is particularly relevant in CI/CD scenarios where Allure results may be generated by untrusted code (e.g., from a pull request).
Consider validating attachment sources at the Zod schema level:
| // Collect attachment file paths | |
| const attachmentPaths: string[] = [] | |
| if (result.attachments) { | |
| for (const attachment of result.attachments) { | |
| attachmentPaths.push(attachment.source) | |
| } | |
| } | |
| const attachmentPaths: string[] = [] | |
| if (result.attachments) { | |
| for (const attachment of result.attachments) { | |
| // Reject paths with directory traversal components | |
| if (attachment.source.includes('..') || attachment.source.includes('/') || attachment.source.includes('\\')) { | |
| console.warn(`Warning: Skipping attachment with suspicious path "${attachment.source}" in ${file}`) | |
| continue | |
| } | |
| attachmentPaths.push(attachment.source) | |
| } | |
| } |
Note: the same path traversal concern exists in the existing JUnit XML and Playwright JSON parsers via their attachment path extraction, so this is a pre-existing architectural gap. But worth addressing here while adding new code.
| options: ParserOptions | ||
| ): Promise<TestCaseResult[]> => { | ||
| const dirPath = data | ||
| const entries = readdirSync(dirPath) |
There was a problem hiding this comment.
No error handling for non-existent directory (Medium)
readdirSync(dirPath) will throw a raw ENOENT error with a Node.js stack trace if the user provides a non-existent directory path. Unlike the JUnit/Playwright parsers (which receive pre-read file content from ResultUploadCommandHandler), the Allure parser reads the filesystem directly, so directory-not-found errors hit here unhandled.
| const entries = readdirSync(dirPath) | |
| const entries = (() => { | |
| try { | |
| return readdirSync(dirPath) | |
| } catch { | |
| throw new Error(`Could not read Allure results directory: ${dirPath}`) | |
| } | |
| })() |
|
|
||
| // Fall back to regex on link name | ||
| if (link.name) { | ||
| const match = link.name.match(/\b[A-Z]+-\d+\b/) |
There was a problem hiding this comment.
TMS link name regex is broader than the project code pattern used elsewhere (Low)
/\b[A-Z]+-\d+\b/ matches any uppercase-letters-dash-digits pattern like HTTP-200, SHA-256, or V-1. The rest of the codebase (ResultUploadCommandHandler.detectProjectCodeFromTCaseNames) uses a stricter pattern: [A-Za-z0-9]{1,5} for the project code and \d{3,} for at least 3 digits.
Consider aligning to reduce false positives:
| const match = link.name.match(/\b[A-Z]+-\d+\b/) | |
| const match = link.name.match(/\b[A-Z]{1,5}-\d{3,}\b/) |
| const result = parsed.data | ||
| const status = mapAllureStatus(result.status) | ||
| const folder = getFolderFromLabels(result.labels) | ||
| const duration = result.stop - result.start |
There was a problem hiding this comment.
No guard against negative duration from corrupted timestamps (Low)
If an Allure result file has stop < start (corrupted data), this produces a negative duration. The JUnit parser guards against this with Number.isFinite(timeTakenSeconds) && timeTakenSeconds >= 0, returning null for invalid values.
| const duration = result.stop - result.start | |
| const duration = result.stop - result.start | |
| const timeTaken = duration >= 0 ? duration : null |
Then use timeTaken instead of duration when constructing the test case result on line 216.
| labels: allureLabelSchema.array().nullable().optional(), | ||
| links: allureLinkSchema.array().nullable().optional(), | ||
| parameters: allureParameterSchema.array().nullable().optional(), | ||
| steps: z.any().nullable().optional(), |
There was a problem hiding this comment.
Prefer z.unknown() over z.any() for type safety (Nit)
z.any() infers as TypeScript any, which disables type checking. Since the steps field is not used by the parser, z.unknown() would be safer -- it infers as unknown and requires explicit narrowing before use.
| steps: z.any().nullable().optional(), | |
| steps: z.unknown().nullable().optional(), |
| const testcases = await parseAllureResults(allureBasePath, allureBasePath, defaultOptions) | ||
|
|
||
| // Should only parse *-result.json files, not containers or attachments | ||
| expect(testcases.length).toBeGreaterThanOrEqual(12) |
There was a problem hiding this comment.
Weak assertion on fixture count (Nit)
The unit-tests directory contains exactly 12 *-result.json files, so this should assert an exact count to catch accidental duplicates or missing parses. The JUnit and Playwright unit tests both assert exact counts.
| expect(testcases.length).toBeGreaterThanOrEqual(12) | |
| expect(testcases).toHaveLength(12) |
1 participant
Summary
allure-uploadCLI command that parses Allure results directories (*-result.jsonfiles) and uploads test results to QA SphereTest plan
🤖 Generated with Claude Code