-
Notifications
You must be signed in to change notification settings - Fork 0
Add support for creating test cases #39
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
base: main
Are you sure you want to change the base?
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 test result upload commands ( Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
dfe9d22 to
993ce44
Compare
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 a significant enhancement to the junit-upload and playwright-json-upload commands, allowing users to automatically create new test cases in QA Sphere for results that lack valid markers. The README.md has been updated to clarify the two modes of operation (uploading to an existing run or creating a new one) and details the new --project-code and --create-tcases options, including examples and updated requirements for test case matching. Core API changes include new interfaces for pagination and test case creation (PaginatedResponse, TCase, CreateTCasesRequest, CreateTCasesResponse) in src/api/schemas.ts, and corresponding API methods (getTCasesPaginated, createTCases) in src/api/tcases.ts. A new utility appendSearchParams was added in src/api/utils.ts for building URL query parameters. The command-line interface in src/commands/resultUpload.ts now includes the --project-code and --create-tcases options, with revised examples and epilogue text. New test fixtures were added, and src/tests/result-upload.spec.ts was updated with new test cases and mocked API handlers to validate the test case creation functionality, including cleanup for generated mapping files. The ResultUploadCommandHandler in src/utils/result-upload/ResultUploadCommandHandler.ts was refactored to support the new modes, including a discriminated union for command arguments, logic for detecting project codes, fetching existing test cases, creating new ones, and generating a mapping file for newly created test cases. Review comments highlighted a type safety issue with the runName property in ResultUploadCommandArgs, an unsafe type assertion for runName in processTemplate, and a potential bug in the isValidValue utility function where 0 was incorrectly treated as an invalid value, suggesting a more robust check for null and undefined.
| const isValidValue = (value: unknown) => { | ||
| return value || value === false || value === '' | ||
| } |
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 isValidValue function currently filters out the number 0 because 0 is a falsy value. This could lead to unexpected behavior if an API parameter legitimately needs to be 0. A safer and clearer implementation would be to explicitly check for null and undefined.
| const isValidValue = (value: unknown) => { | |
| return value || value === false || value === '' | |
| } | |
| const isValidValue = (value: unknown) => { | |
| return value !== null && value !== undefined | |
| } |
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.
seems logical
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.
Great work!
| fetcher(`/api/public/v0/project/${projectCode}/tcase/seq`, { | ||
| method: 'POST', | ||
| body: JSON.stringify(request), | ||
| }).then((r) => jsonResponse<PaginatedResponse<TCase>>(r)), |
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 add zod for validation in future.
| const isValidValue = (value: unknown) => { | ||
| return value || value === false || value === '' | ||
| } |
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.
seems logical
| const tcaseRefs = new Set<string>() | ||
| protected async getTCaseIds(projectCode: string, fileResults: FileResults[]) { | ||
| const shouldFailOnInvalid = !this.args.force && !this.args.ignoreUnmatched | ||
| const tcaseSeqRegex = new RegExp(`${projectCode}-(\\d{3,})`) |
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 can reduce false positives if we give priority to matches that start at the beginning or at the end somehow.
| const response = await this.api.folders.getFoldersPaginated(projectCode, { | ||
| page, | ||
| limit: DEFAULT_PAGE_SIZE, | ||
| }) | ||
|
|
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.
In future, We can introduce folder search by folder title. Right now we are overfetching
| const mappingFilename = processTemplate(DEFAULT_MAPPING_FILENAME_TEMPLATE) | ||
| const mappingLines = tcases | ||
| .map((t, i) => `${getTCaseMarker(projectCode, t.seq)}: ${tcasesToCreate[i]}`) | ||
| .join('\n') |
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.
If we use this mapping file created via a previous use, we could avoid the step of fetching test cases from default folder and then matching the titles and all matches are found.
This fixes https://github.com/Hypersequent/tms-issues/issues/2141