-
Notifications
You must be signed in to change notification settings - Fork 0
Add support for uploading xcresult bundle #40
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: dev/himanshu/create-tcase-flag
Are you sure you want to change the base?
Add support for uploading xcresult bundle #40
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 introduces comprehensive support for uploading Xcode result bundles ( 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
|
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 adds support for uploading .xcresult bundles from Xcode, which is a great new feature. The implementation includes a new parser for the .xcresult format, leveraging better-sqlite3 to read the embedded database, which is a solid approach. The necessary changes are propagated throughout the CLI, including command registration, documentation, and testing. The refactoring of existing test parsers to accept file paths instead of content is a good move for consistency.
I have a couple of suggestions for the new xcresult parser to improve its robustness and performance. Specifically, I've pointed out the use of execSync in an async function and a potentially brittle way of parsing test names.
Overall, this is a well-executed feature addition. Great work!
| execSync(`xcrun xcresulttool get test-results summary --path "${bundlePath}"`, { | ||
| stdio: 'ignore', | ||
| }) |
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 use of execSync blocks the Node.js event loop, which is an anti-pattern in an async function. This can freeze UI elements like spinners. It's better to use the asynchronous exec instead, especially since parseXCResult is already an async function. You can use dynamic imports and promisify to achieve this.
| execSync(`xcrun xcresulttool get test-results summary --path "${bundlePath}"`, { | |
| stdio: 'ignore', | |
| }) | |
| const { promisify } = await import('node:util'); | |
| const { exec } = await import('node:child_process'); | |
| await promisify(exec)(`xcrun xcresulttool get test-results summary --path "${bundlePath}"`); |
| ) | ||
|
|
||
| results.push({ | ||
| name: (testCase.name ?? 'Unknown Test').split('(')[0], // Names include "()" 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.
Using split('(')[0] to clean the test case name is a bit brittle. It's meant to remove the test suite context that Xcode sometimes appends (e.g., testMyFeature(MyTests)), but it will also truncate valid test names that contain parentheses for other reasons, like testSomething(withContext). A regular expression would be more robust for removing only a parenthesized suffix.
| name: (testCase.name ?? 'Unknown Test').split('(')[0], // Names include "()" as well | |
| name: (testCase.name ?? 'Unknown Test').replace(/\(.*\)$/, '').trim(), // Names include "()" 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.
Need to configure prettier to run automatically in pre commit hook
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.
Changes because parsePlaywrightJson now expects file name instead of contents
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.
Changes because parseJUnitXml now expects file name instead of contents
|
|
||
| if (result.retry) { | ||
| message += `<p><b>Test passed in ${result.retry + 1} attempts</b></p>` | ||
| message += `<p><strong>Test passed in ${result.retry + 1} attempts</strong></p>` |
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.
Backend expects strong tag instead of b
| protected detectProjectCodeFromTCaseNames(fileResults: FileResults[]) { | ||
| // Look for pattern like PRJ-123 or TEST-456 | ||
| const tcaseSeqRegex = new RegExp(/([A-Za-z0-9]{1,5})-\d{3,}/) | ||
| // Look for pattern like PRJ-123 or TEST-456 (_ is also allowed as separator) |
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.
_ because function names in xcresult files can't have -
| printError( | ||
| `Test case name "${result.name}" in ${file} does not contain valid test case marker` | ||
| ) | ||
| commandTypePrintMissingMarkerGuidance[this.type](projectCode, result.name) |
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.
Added marker guidelines here 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.
Can be tested using xcresult bundle added in the test or from https://github.com/Hypersequent/bistro-ios-xct
|
|
||
| export const parseXCResult: Parser = async (bundlePath: string): Promise<TestCaseResult[]> => { | ||
| const dbPath = path.join(bundlePath, sqliteFile) | ||
| if (!existsSync(dbPath)) { |
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.
After inspecting the demo bundles present in https://github.com/Hypersequent/bistro-ios-xct, I expected the sqlite files to be always present in the bundle. However, when I myself generated new bundles (after implementation), it didn't contain the sqlite file.
Found that we need to run some xcrun xcresulttool command, which then generates the sqlite file (on the first run). So now this becomes a dependency
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.
This bundle is same as the one at https://github.com/Hypersequent/bistro-ios-xct/tree/main/Saved%20Results. I only retained the sqlite file as that is sufficient for our case
- Updated tcase names to contain valid markers like TEST_002
- Deleted entries from attachments table. Then we won't need the raw files present in data directory
This fixes https://github.com/Hypersequent/tms-issues/issues/2142