test: enable test shard #839
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
There was a problem hiding this comment.
Pull Request Overview
This PR enables test sharding in GitHub Actions to parallelize test execution across multiple workers, reducing CI build time by distributing tests across 5 shards for the main test jobs.
Key changes:
- Updated
@eggjs/scriptsdependency from fixed version to beta to support test sharding - Added test sharding configuration with 5 parallel workers for PostgreSQL and MySQL test jobs
- Added environment variables to enable test shard coordination
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| package.json | Updated @eggjs/scripts to beta version to support test sharding functionality |
| .github/workflows/nodejs.yml | Added test sharding matrix configuration and environment variables for parallel test execution |
Summary of ChangesHello @fengmk2, 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 primarily focuses on updating a key development dependency, Highlights
Ignored Files
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
|
|
Warning Rate limit exceeded@fengmk2 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 53 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a CI typecheck job and converts several test jobs to shard-aware matrices with CI_NODE_INDEX/CI_NODE_TOTAL and shard-aware naming (one gated to master); simplifies npm test scripts and bumps Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant GH as GitHub Actions
participant Type as Typecheck Job
participant Shard as Sharded Test Jobs
Dev->>GH: Push / Open PR
GH->>Type: checkout → setup-node(22) → install → lint → typecheck → build
par Sharded tests
GH->>Shard: Start matrix shards (node-version/os, shardIndex/shardTotal)
Shard->>Shard: set env CI_NODE_INDEX / CI_NODE_TOTAL
Shard-->>GH: Report shard result
end
GH-->>Dev: CI statuses per job/shard
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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. Comment |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Code Review
This pull request updates the @eggjs/scripts dependency to use the beta tag, presumably to enable test sharding. While this may enable new functionality, using a distribution tag like beta introduces risks to build reproducibility and stability. My review comment suggests pinning the dependency to a specific pre-release version to mitigate these risks.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
app/port/controller/package/SavePackageVersionController.ts (1)
120-121: Consider using a local variable instead of reassigning the parameter.While the lint suppression is valid, reassigning function parameters can make code harder to reason about. Consider using a local variable instead.
Apply this diff to refactor:
- // oxlint-disable-next-line no-param-reassign - fullname = fullname.trim(); + const trimmedFullname = fullname.trim(); - if (fullname !== pkg.name) { + if (trimmedFullname !== pkg.name) { throw new UnprocessableEntityError( - `fullname(${fullname}) not match package.name(${pkg.name})` + `fullname(${trimmedFullname}) not match package.name(${pkg.name})` ); }Then update all subsequent references to
fullnameto usetrimmedFullnameinstead (lines 132, 182, etc.).app/port/controller/PackageVersionFileController.ts (1)
168-169: Consider using a local variable instead of reassigning the parameter.While the lint suppression is valid, reassigning function parameters can make code harder to reason about. Consider using a local variable instead.
Apply this diff to refactor:
- // oxlint-disable-next-line no-param-reassign - path = `/${path}`; + const absolutePath = `/${path}`; const packageVersion = await this.#getPackageVersion( ctx, fullname, scope, name, versionSpec ); - if (path.endsWith('/')) { - const directory = path.slice(0, -1); + if (absolutePath.endsWith('/')) { + const directory = absolutePath.slice(0, -1); const files = await this.#listFilesByDirectory(packageVersion, directory);Then update all subsequent references to
pathto useabsolutePathinstead (lines 177, 179, 194, 196, 203, 206, 212).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.oxlintrc.json(1 hunks)app/common/adapter/binary/FirefoxBinary.ts(1 hunks)app/common/adapter/changesStream/CnpmjsorgChangesStream.ts(1 hunks)app/core/service/CreateHookTriggerService.ts(1 hunks)app/core/service/FixNoPaddingVersionService.ts(1 hunks)app/port/controller/PackageVersionFileController.ts(1 hunks)app/port/controller/package/SavePackageVersionController.ts(1 hunks)app/repository/util/ModelConvertor.ts(2 hunks)test/common/adapter/binary/ChromeForTestingBinary.test.ts(1 hunks)test/common/adapter/binary/EdgedriverBinary.test.ts(1 hunks)tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- test/common/adapter/binary/ChromeForTestingBinary.test.ts
- test/common/adapter/binary/EdgedriverBinary.test.ts
- app/core/service/FixNoPaddingVersionService.ts
- app/core/service/CreateHookTriggerService.ts
🧰 Additional context used
📓 Path-based instructions (2)
app/port/controller/**/*.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place HTTP controllers under app/port/controller/
Files:
app/port/controller/PackageVersionFileController.tsapp/port/controller/package/SavePackageVersionController.ts
app/repository/**/*.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place data access repositories under app/repository/
Files:
app/repository/util/ModelConvertor.ts
🧬 Code graph analysis (1)
app/repository/util/ModelConvertor.ts (1)
app/repository/PackageRepository.ts (1)
model(230-251)
🪛 Biome (2.1.2)
.oxlintrc.json
[error] 19-19: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 19-19: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 19-19: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 19-19: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 20-20: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 20-20: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 20-20: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
⏰ 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: test on mysql (node@20, shard@2/3)
- GitHub Check: test on mysql (node@22, shard@2/3)
- GitHub Check: test on mysql (node@22, shard@0/3)
- GitHub Check: test on mysql (node@24, shard@0/3)
- GitHub Check: test on mysql (node@20, shard@1/3)
- GitHub Check: test on postgresql (node@24, shard@2/3)
- GitHub Check: test on mysql (node@22, shard@1/3)
- GitHub Check: test on postgresql (node@24, shard@1/3)
- GitHub Check: test on postgresql (node@20, shard@1/3)
- GitHub Check: test on postgresql (node@24, shard@0/3)
- GitHub Check: test on postgresql (node@22, shard@1/3)
- GitHub Check: test on postgresql (node@20, shard@2/3)
- GitHub Check: test on postgresql (node@22, shard@0/3)
- GitHub Check: test on postgresql (node@22, shard@2/3)
- GitHub Check: test on postgresql (node@20, shard@0/3)
- GitHub Check: test-mysql57-s3-nfs (20, ubuntu-latest)
- GitHub Check: test-mysql57-s3-nfs (22, ubuntu-latest)
- GitHub Check: build-and-push-image
🔇 Additional comments (5)
tsconfig.json (1)
4-4: LGTM! ES2022 target enables modern syntax.Updating the target to ES2022 enables
Array.prototype.at()and other modern features used throughout this PR. The change is appropriate and aligns with the codebase modernization effort.app/common/adapter/binary/FirefoxBinary.ts (1)
94-95: LGTM! Clean refactor with proper fallback.The change to
parts.at(-1) ?? ''is more readable thanparts[parts.length - 1]. The?? ''fallback correctly handles the edge case wherehrefis just'/'(resulting in an emptypartsarray).app/common/adapter/changesStream/CnpmjsorgChangesStream.ts (1)
56-57: LGTM! Defensive refactor with modern syntax.The changes to use
results.at(-1)and optional chaininglast?.gmt_modifiedare appropriate. While the guard condition at line 54 (results?.length >= limit) ensures the array is non-empty, the optional chaining adds defensive safety without any downside..oxlintrc.json (1)
19-20: LGTM!The lint rule additions are appropriate and align with the inline suppressions added in the controller files.
Note: The static analysis hints from Biome are false positives—Biome is attempting to parse an oxlint configuration file, which uses a different schema.
app/repository/util/ModelConvertor.ts (1)
120-136: Excellent refactor to eliminate input mutation!The change from reassigning the
dataparameter to creating a localentityDatavariable is a significant improvement. This prevents side effects by ensuring the original input object is never mutated, which aligns with functional programming principles and makes the code more predictable.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #839 +/- ##
==========================================
- Coverage 95.31% 95.16% -0.15%
==========================================
Files 197 197
Lines 23904 23906 +2
Branches 2188 2301 +113
==========================================
- Hits 22784 22751 -33
- Misses 1120 1155 +35 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/core/service/FixNoPaddingVersionService.ts (1)
24-27: Use explicit bigint conversion instead ofas unknown as number.Replace the double‐cast with one of the following, depending on whether you need a number or a bigint:
// to number id = Number(lastVersion.id) + 1; // or to stay in bigint id = lastVersion.id + 1n;Adjust the type of
idaccordingly.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.oxlintrc.json(1 hunks)app/common/adapter/binary/FirefoxBinary.ts(1 hunks)app/common/adapter/changesStream/CnpmjsorgChangesStream.ts(1 hunks)app/core/service/FixNoPaddingVersionService.ts(1 hunks)tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/common/adapter/changesStream/CnpmjsorgChangesStream.ts
- tsconfig.json
🧰 Additional context used
📓 Path-based instructions (1)
app/core/service/**/*.ts
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Place business logic services under app/core/service/
Files:
app/core/service/FixNoPaddingVersionService.ts
🪛 Biome (2.1.2)
.oxlintrc.json
[error] 19-19: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 19-19: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 19-19: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 19-19: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 20-20: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 20-20: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 20-20: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 20-20: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 21-21: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 21-21: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
[error] 21-21: End of file expected
Use an array for a sequence of values: [1, 2]
(parse)
⏰ 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). (20)
- GitHub Check: test on mysql (node@24, shard@2/3)
- GitHub Check: test on postgresql (node@24, shard@0/3)
- GitHub Check: test on mysql (node@24, shard@0/3)
- GitHub Check: test on mysql (node@24, shard@1/3)
- GitHub Check: test on postgresql (node@22, shard@1/3)
- GitHub Check: test on mysql (node@22, shard@2/3)
- GitHub Check: test on mysql (node@20, shard@1/3)
- GitHub Check: test on postgresql (node@20, shard@1/3)
- GitHub Check: test on mysql (node@22, shard@0/3)
- GitHub Check: test on postgresql (node@20, shard@2/3)
- GitHub Check: test on postgresql (node@24, shard@1/3)
- GitHub Check: test on postgresql (node@24, shard@2/3)
- GitHub Check: test on mysql (node@20, shard@0/3)
- GitHub Check: test on mysql (node@22, shard@1/3)
- GitHub Check: test on postgresql (node@22, shard@2/3)
- GitHub Check: test on postgresql (node@22, shard@0/3)
- GitHub Check: test on postgresql (node@20, shard@0/3)
- GitHub Check: test-mysql57-s3-nfs (20, ubuntu-latest)
- GitHub Check: test-mysql57-s3-nfs (22, ubuntu-latest)
- GitHub Check: build-and-push-image
🔇 Additional comments (2)
.oxlintrc.json (1)
19-21: LGTM!The lint rule relaxations are appropriate for project-specific needs. The Biome parse errors flagged in static analysis are false positives—JSON5 comments are valid in oxlint configuration files.
app/common/adapter/binary/FirefoxBinary.ts (1)
95-95: LGTM!The nullish coalescing operator provides a safe fallback when
partsis empty (e.g., whenhrefis/or contains only separators). This defensive approach prevents potential undefined values from propagating.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.docker/build.sh(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Create and publish a Docker image
.docker/build.sh
[error] 1-1: Command failed with exit code 1: /bin/sh -c .docker/build.sh
⏰ 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: test on mysql (node@24, shard@0/3)
- GitHub Check: test on postgresql (node@24, shard@2/3)
- GitHub Check: test on postgresql (node@24, shard@1/3)
- GitHub Check: test on postgresql (node@22, shard@1/3)
- GitHub Check: test on postgresql (node@22, shard@2/3)
- GitHub Check: test on postgresql (node@20, shard@0/3)
- GitHub Check: test on mysql (node@24, shard@1/3)
- GitHub Check: test on postgresql (node@22, shard@0/3)
- GitHub Check: test on postgresql (node@24, shard@0/3)
- GitHub Check: test on mysql (node@24, shard@2/3)
- GitHub Check: test on postgresql (node@20, shard@2/3)
- GitHub Check: test on postgresql (node@20, shard@1/3)
- GitHub Check: test on mysql (node@22, shard@1/3)
- GitHub Check: test on mysql (node@22, shard@0/3)
- GitHub Check: test on mysql (node@20, shard@1/3)
- GitHub Check: test on mysql (node@20, shard@2/3)
- GitHub Check: test on mysql (node@22, shard@2/3)
- GitHub Check: test on mysql (node@20, shard@0/3)
.docker/build.sh
Outdated
| node -v && npm -v \ | ||
| && npm install -g npminstall --registry=https://registry.npmmirror.com \ | ||
| && (npminstall -c || true) \ |
There was a problem hiding this comment.
Don’t gate the build on npm install -g npminstall.
Chaining everything with && means a transient/global install failure now aborts the script before (npminstall -c || true) can fall back, whereas previously the script kept going. The CI Docker build is already failing with exit 1 because of this regression. Please either drop the && before this step or wrap it in a tolerant guard (e.g., (npm install -g … || true)), so the build remains resilient.
🤖 Prompt for AI Agents
In .docker/build.sh around lines 3 to 5, the script currently uses "&& npm
install -g npminstall ..." which gates the rest of the build on a
potentially-failing global install; change this to be tolerant so transient
failures don't abort the script: either remove the preceding "&&" so the command
is not required to succeed for subsequent commands, or wrap the install in a
tolerant guard like "(npm install -g npminstall --registry=... || true)" so the
failure is ignored and execution continues to the fallback "(npminstall -c ||
true)".
[skip ci] ## 4.10.0 (2025-10-15) * feat: Add time field to abbreviated metadata for pnpm time-based resolution (#834) ([2e51399](2e51399)), closes [#834](#834) * feat: mirror microsoft/ripgrep-prebuilt binary (#842) ([606c983](606c983)), closes [#842](#842) [#841](#841) * docs: Add comprehensive internal API documentation for direct HTTP requests (#832) ([5b1da74](5b1da74)), closes [#832](#832) [#709](#709) * docs: Enhance Copilot instructions with DDD architecture [skip ci] (#838) ([cb93141](cb93141)), closes [#838](#838) [#837](#837) * test: enable test shard (#839) ([324511d](324511d)), closes [#839](#839) * refactor: use tegg and egg v4 beta (#836) ([c7df471](c7df471)), closes [#836](#836)
|
🎉 This PR is included in version 4.10.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary by CodeRabbit
New Features
Bug Fixes
Chores