Conversation
📝 WalkthroughWalkthroughThe Jest setup now unconditionally polyfills Changes
Sequence Diagram(s)sequenceDiagram
participant TestSuite as Cloud Runner Github Checks Unit Tests
participant GitHubAPI as GitHub API Module
TestSuite->>GitHubAPI: Mock createGitHubCheckRequest
TestSuite->>GitHubAPI: Mock updateGitHubCheckRequest
TestSuite->>GitHubAPI: Mock runUpdateAsyncChecksWorkflow
TestSuite->>GitHubAPI: Run tests using mocked methods
TestSuite->>GitHubAPI: Restore original methods after each test
sequenceDiagram
participant GitHubActions as GitHub Actions Workflow
participant Runner as Test Runner
participant GitHub as GitHub API
GitHubActions->>Runner: Trigger githubChecksIntegration job (if runGithubIntegrationTests = true)
Runner->>GitHub: Run real integration tests (create & update GitHub checks)
GitHub-->>Runner: Return check IDs and statuses
Runner-->>GitHubActions: Report test results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/jest.setup.ts (1)
12-19: Remove unnecessary ESLint disable comments.The
@typescript-eslint/no-explicit-anydisable comments are flagged as unused by ESLint, suggesting TypeScript isn't reporting errors for these type assertions in this context.- // eslint-disable-next-line @typescript-eslint/no-explicit-any (global as any).fetch = fetch; - // eslint-disable-next-line @typescript-eslint/no-explicit-any (global as any).Headers = Headers; - // eslint-disable-next-line @typescript-eslint/no-explicit-any (global as any).Request = Request; - // eslint-disable-next-line @typescript-eslint/no-explicit-any (global as any).Response = Response;src/model/cloud-runner/tests/cloud-runner-github-checks.test.ts (1)
30-32: Fix formatting and remove useless undefined.Address the Prettier formatting and ESLint issues flagged by static analysis.
- jest - .spyOn(GitHub as any, 'runUpdateAsyncChecksWorkflow') - .mockResolvedValue(undefined); + jest.spyOn(GitHub as any, 'runUpdateAsyncChecksWorkflow').mockResolvedValue();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/jest.setup.ts(1 hunks)src/model/cloud-runner/tests/cloud-runner-github-checks.test.ts(1 hunks)
🧰 Additional context used
🪛 ESLint
src/jest.setup.ts
[error] 12-12: '@typescript-eslint/no-explicit-any' rule is disabled but never reported.
(eslint-comments/no-unused-disable)
[error] 14-14: '@typescript-eslint/no-explicit-any' rule is disabled but never reported.
(eslint-comments/no-unused-disable)
[error] 16-16: '@typescript-eslint/no-explicit-any' rule is disabled but never reported.
(eslint-comments/no-unused-disable)
[error] 18-18: '@typescript-eslint/no-explicit-any' rule is disabled but never reported.
(eslint-comments/no-unused-disable)
src/model/cloud-runner/tests/cloud-runner-github-checks.test.ts
[error] 30-32: Replace ⏎········.spyOn(GitHub·as·any,·'runUpdateAsyncChecksWorkflow')⏎········ with .spyOn(GitHub·as·any,·'runUpdateAsyncChecksWorkflow')
(prettier/prettier)
[error] 32-32: Do not use useless undefined.
(unicorn/no-useless-undefined)
🪛 GitHub Actions: Integrity
src/model/cloud-runner/tests/cloud-runner-github-checks.test.ts
[warning] 1-1: Prettier formatting check failed. Code style issues found. Forgot to run Prettier?
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: iOS on 2022.3.13f1
- GitHub Check: A cat for your effort!
- GitHub Check: WebGL on 2022.3.13f1
- GitHub Check: StandaloneWindows64 on 2023.2.2f1
- GitHub Check: StandaloneOSX on 2023.2.2f1
- GitHub Check: WebGL on 2021.3.32f1
- GitHub Check: StandaloneOSX on 2022.3.13f1
- GitHub Check: StandaloneWindows64 on 2021.3.32f1
- GitHub Check: iOS on 2021.3.32f1
- GitHub Check: Android on 2021.3.32f1
- GitHub Check: StandaloneOSX on 2021.3.32f1
- GitHub Check: StandaloneLinux64 on 2021.3.32f1
- GitHub Check: StandaloneLinux64 on 2021.3.32f1
- GitHub Check: StandaloneLinux64 on 2021.3.32f1
- GitHub Check: StandaloneWindows64 on 2021.3.32f1
🔇 Additional comments (3)
src/jest.setup.ts (1)
3-21: LGTM! Clean polyfill implementation for Jest testing.The conditional polyfill for fetch API using undici is well-implemented and properly documented. This addresses the common issue where Jest's VM context lacks fetch globals even on Node 18+.
src/model/cloud-runner/tests/cloud-runner-github-checks.test.ts (2)
20-37: Good test isolation with GitHub API mocking.The mocking setup effectively prevents real network calls during tests and provides controlled responses. The conditional application based on
cloudRunnerDebugis appropriate.
1-79: Formatting issues resolved via PrettierI’ve run
npx prettier --write src/model/cloud-runner/tests/cloud-runner-github-checks.test.ts, which fixed the formatting errors that were causing the pipeline to fail. Please commit the updated file to restore CI green status.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/model/cloud-runner/tests/cloud-runner-github-checks.test.ts (1)
22-29: Streamline stubbing viajest.mockThree repeated
spyOncalls can be replaced with a single module mock, reducing repetition and easing future maintenance.jest.mock('../../github', () => ({ createGitHubCheckRequest: jest.fn().mockResolvedValue({ status: 201, data: { id: '1' } }), updateGitHubCheckRequest: jest.fn().mockResolvedValue({ status: 200, data: {} }), runUpdateAsyncChecksWorkflow: jest.fn().mockResolvedValue(), }));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/model/cloud-runner/tests/cloud-runner-github-checks.test.ts(1 hunks)
🧰 Additional context used
🪛 ESLint
src/model/cloud-runner/tests/cloud-runner-github-checks.test.ts
[error] 30-30: Do not use useless undefined.
(unicorn/no-useless-undefined)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
- GitHub Check: Android on 2021.3.32f1
- GitHub Check: StandaloneLinux64 on 2023.2.2f1
- GitHub Check: Android on 2022.3.13f1
- GitHub Check: iOS on 2022.3.13f1
- GitHub Check: StandaloneWindows64 on 2022.3.13f1
- GitHub Check: Android on 2021.3.32f1
- GitHub Check: iOS on 2021.3.32f1
- GitHub Check: StandaloneLinux64 on 2022.3.13f1
- GitHub Check: StandaloneLinux64 on 2021.3.32f1
- GitHub Check: StandaloneLinux64 on 2021.3.32f1
- GitHub Check: StandaloneOSX on 2021.3.32f1
- GitHub Check: StandaloneWindows64 on 2021.3.32f1
- GitHub Check: StandaloneLinux64 on 2021.3.32f1
- GitHub Check: StandaloneWindows64 on 2021.3.32f1
- GitHub Check: iOS on 2023.2.2f1
- GitHub Check: StandaloneOSX on 2022.3.13f1
- GitHub Check: iOS on 2021.3.45f1
- GitHub Check: iOS on 2022.3.13f1
- GitHub Check: StandaloneOSX on 2021.3.45f1
| status: 200, | ||
| data: {}, | ||
| }); | ||
| jest.spyOn(GitHub as any, 'runUpdateAsyncChecksWorkflow').mockResolvedValue(undefined); |
There was a problem hiding this comment.
mockResolvedValue(undefined) triggers ESLint unicorn/no-useless-undefined
Omit the argument—mockResolvedValue() already resolves with undefined.
-jest.spyOn(GitHub as any, 'runUpdateAsyncChecksWorkflow').mockResolvedValue(undefined);
+jest.spyOn(GitHub as any, 'runUpdateAsyncChecksWorkflow').mockResolvedValue();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| jest.spyOn(GitHub as any, 'runUpdateAsyncChecksWorkflow').mockResolvedValue(undefined); | |
| jest.spyOn(GitHub as any, 'runUpdateAsyncChecksWorkflow').mockResolvedValue(); |
🧰 Tools
🪛 ESLint
[error] 30-30: Do not use useless undefined.
(unicorn/no-useless-undefined)
🤖 Prompt for AI Agents
In src/model/cloud-runner/tests/cloud-runner-github-checks.test.ts at line 30,
the call to mockResolvedValue(undefined) triggers an ESLint
unicorn/no-useless-undefined warning because mockResolvedValue() without
arguments already resolves with undefined. Fix this by removing the undefined
argument and just use mockResolvedValue() instead.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #723 +/- ##
==========================================
+ Coverage 37.05% 38.22% +1.16%
==========================================
Files 77 77
Lines 3163 3163
Branches 626 662 +36
==========================================
+ Hits 1172 1209 +37
+ Misses 1991 1809 -182
- Partials 0 145 +145 🚀 New features to boost your workflow:
|
frostebite
requested review from
AndrewKahr,
GabLeRoux,
cloudymax,
davidmfinol and
lazerwalker
|
Seems reasonable, but since my TS is not very strong I would need to ask:
|
frostebite
requested review from
EricRibeiro,
KyleTryon,
MichaelBuhler and
jcs090218
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/jest.setup.ts (1)
3-11: LGTM! Excellent solution for Jest fetch polyfill.The fetch polyfill implementation correctly addresses the issue where Jest's VM context lacks the fetch API even on Node 18+. The approach of using undici's implementation and assigning it to
globalThisis the standard pattern for this scenario.The ESLint error about
globalThisnot being defined is a false positive -globalThisis available in Node.js 12+ and all modern environments. Consider addingglobalThisto your ESLint globals configuration to resolve this warning.To resolve the ESLint warning, you can add this to your ESLint configuration:
{ "globals": { + "globalThis": "readonly" } }src/integration/cloud-runner-github-checks.integration.test.ts (2)
31-34: Consider cleanup of created GitHub checks.The test creates real GitHub checks but doesn't clean them up afterward. Over time, this could accumulate test artifacts in the GitHub repository.
Consider adding cleanup logic:
const checkId = await GitHub.createGitHubCheck(`integration create`); expect(checkId).not.toEqual(''); await GitHub.updateGitHubCheck(`1 ${new Date().toISOString()}`, `integration`); await GitHub.updateGitHubCheck(`2 ${new Date().toISOString()}`, `integration`, `success`, `completed`); + + // Optional: Clean up the created check if the GitHub API supports it + // await GitHub.deleteGitHubCheck?.(checkId);
36-36: Consider using a more reasonable timeout.The infinite timeout (1e9 ms ≈ 11.5 days) could mask actual issues and prevent proper test failure detection.
Consider using a more reasonable timeout for integration tests:
- TIMEOUT_INFINITE, + 300000, // 5 minutes - adjust based on expected GitHub API response times
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/cloud-runner-ci-pipeline.yml(2 hunks)src/integration/cloud-runner-github-checks.integration.test.ts(1 hunks)src/jest.setup.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: the build steps duplication in the workflow is intentional to implement a retry mechanism with diffe...
Learnt from: MichaelBuhler
PR: game-ci/unity-builder#685
File: .github/workflows/build-tests-ubuntu.yml:146-146
Timestamp: 2025-01-30T18:01:50.339Z
Learning: The build steps duplication in the workflow is intentional to implement a retry mechanism with different delay intervals between attempts.
Applied to files:
.github/workflows/cloud-runner-ci-pipeline.yml
🪛 GitHub Check: Tests
src/integration/cloud-runner-github-checks.integration.test.ts
[failure] 1-1:
Filename 'cloud-runner-github-checks.integration.test.ts' does not match the naming convention
🪛 ESLint
src/integration/cloud-runner-github-checks.integration.test.ts
[error] 1-39: Filename 'cloud-runner-github-checks.integration.test.ts' does not match the naming convention.
(filenames/match-regex)
src/jest.setup.ts
[error] 11-11: 'globalThis' is not defined.
(no-undef)
🪛 GitHub Actions: Integrity
src/integration/cloud-runner-github-checks.integration.test.ts
[error] 1-1: ESLint: Filename 'cloud-runner-github-checks.integration.test.ts' does not match the naming convention (filenames/match-regex)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (18)
- GitHub Check: StandaloneOSX on 2021.3.32f1
- GitHub Check: StandaloneWindows64 on 2021.3.32f1
- GitHub Check: StandaloneOSX on 2023.2.2f1
- GitHub Check: iOS on 2023.2.2f1
- GitHub Check: StandaloneWindows64 on 2023.2.2f1
- GitHub Check: StandaloneOSX on 2022.3.13f1
- GitHub Check: Android on 2023.2.2f1
- GitHub Check: StandaloneOSX on 2021.3.45f1
- GitHub Check: iOS on 2022.3.13f1
- GitHub Check: tvOS on 2022.3.13f1
- GitHub Check: StandaloneWindows64 on 2023.2.2f1
- GitHub Check: WSAPlayer on 2021.3.32f1
- GitHub Check: StandaloneWindows64 on 2022.3.13f1
- GitHub Check: Android on 2022.3.13f1
- GitHub Check: WSAPlayer on 2022.3.13f1
- GitHub Check: Android on 2021.3.32f1
- GitHub Check: StandaloneWindows64 on 2021.3.32f1
- GitHub Check: tvOS on 2021.3.32f1
🔇 Additional comments (3)
src/integration/cloud-runner-github-checks.integration.test.ts (1)
16-17: LGTM! Clean conditional test execution.The conditional execution logic using environment variables is well-implemented and allows for precise control over when integration tests run.
.github/workflows/cloud-runner-ci-pipeline.yml (2)
6-10: LGTM! Well-designed manual workflow dispatch input.The workflow dispatch input parameter is properly configured with appropriate description, required flag, and default value. This allows for controlled execution of integration tests without affecting regular CI runs.
216-231: LGTM! Excellent integration test job implementation.The
githubChecksIntegrationjob is well-structured with:
- Proper conditional execution based on workflow dispatch input
- Correct environment variable setup for the integration test
- Standard Node.js setup with yarn caching
- Appropriate Jest flags for integration testing
- Proper secret handling for GitHub token authentication
The implementation follows GitHub Actions best practices and integrates seamlessly with the existing CI pipeline.
| @@ -0,0 +1,38 @@ | |||
| import { BuildParameters } from '../model'; | |||
There was a problem hiding this comment.
Fix filename to match naming convention.
The filename violates the project's naming convention as indicated by ESLint and pipeline failures.
Consider renaming the file to match the expected pattern. Based on the error, you may need to use a different naming convention such as:
-cloud-runner-github-checks.integration.test.ts
+cloud-runner-github-checks-integration.test.tsOr check your ESLint configuration for the expected filename pattern and adjust accordingly.
🧰 Tools
🪛 GitHub Check: Tests
[failure] 1-1:
Filename 'cloud-runner-github-checks.integration.test.ts' does not match the naming convention
🪛 ESLint
[error] 1-39: Filename 'cloud-runner-github-checks.integration.test.ts' does not match the naming convention.
(filenames/match-regex)
🪛 GitHub Actions: Integrity
[error] 1-1: ESLint: Filename 'cloud-runner-github-checks.integration.test.ts' does not match the naming convention (filenames/match-regex)
🤖 Prompt for AI Agents
In src/integration/cloud-runner-github-checks.integration.test.ts at line 1, the
filename does not conform to the project's naming conventions causing ESLint and
pipeline failures. Rename the file to match the expected pattern defined in the
ESLint configuration or project guidelines, such as adjusting the order or
format of segments in the filename to comply with the naming rules.
• Why is it needed to disable certain ESLint options? — The only disables are for camelcase where the GitHub API requires snake_case fields, so we suppress the camelCase rule around those API payload keys. • Could mocking the GitHub return values lead to tests passing erroneously if the GitHub API changes? — Mocking keeps unit tests deterministic and offline. To catch breaking API changes, we added an optional integration test (RUN_GITHUB_INTEGRATION_TESTS) that exercises real GitHub checks, providing a safety net for API regressions. |
|
Thanks for the explanation! - Approved 👍 |
2 participants
Very small PR. Improving stability/fix for occasional fail on a test for coordinator/cloud runner.
Mocks a github check API which very occasionally fails.
Summary
Testing
cloudRunnerTests=true node node_modules/jest/bin/jest.js cloud-runner-github-checks --detectOpenHandles --forceExit --runInBandhttps://chatgpt.com/s/cd_688fab13de4c81918a329e14fe7b8284
Summary by CodeRabbit
Summary by CodeRabbit