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.
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.
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.
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.
Changes because parsePlaywrightJson now expects file name instead of contents
There was a problem hiding this comment.
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.
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.
_ 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.
Added marker guidelines here as well
There was a problem hiding this comment.
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.
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.
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
There was a problem hiding this comment.
v12 of better-sqlite3 requires node 20, v11 supports node 18 but doesn't have pre-built binaries for node 24. Since node 18 is already EOL, updated min node version requirement to V20
This fixes https://github.com/Hypersequent/tms-issues/issues/2142