Skip to content

ci(napi): shard NAPI tests #12743

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

ci(napi): shard NAPI tests #12743

wants to merge 7 commits into from
+71 −6

Conversation

overlookmotel
Copy link
Member

@overlookmotel overlookmotel commented Aug 1, 2025 •
edited
Loading

Fix #12435.

Shard NAPI tests, because they're the slowest task in CI.

  • Transformer, minifier, and E2E tests run in one task together, since E2E requires transformer and minifier NPM packages to be built.
  • Parser, oxlint2, and playground each run in their own task.

This split seems fairly decent. Each task is now in the 1m40s - 2m40s range. Previously when they all ran in one task together, it was 7 mins+. This should get PRs through the merge queue a lot faster.

@overlookmotel Graphite App
Copy link
Member Author

overlookmotel commented Aug 1, 2025 •
edited
Loading

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

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.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@overlookmotel overlookmotel mentioned this pull request Aug 1, 2025
Merged
@github-actions github-actions bot added the A-ast Area - AST label Aug 1, 2025
@codspeed-hq CodSpeed HQ
Copy link

codspeed-hq bot commented Aug 1, 2025

CodSpeed Instrumentation Performance Report

Merging #12743 will not alter performance

Comparing 08-01-ci_napi_shard_napi_tests (f5df4aa) with main (6a1c7c6)1

Summary

✅ 34 untouched benchmarks

Footnotes

  1. No successful run was found on 08-01-test_napi_parser_disable_raw_transfer_tests_by_default (ce53949) during the generation of this report, so main (6a1c7c6) was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

@overlookmotel overlookmotel marked this pull request as ready for review August 1, 2025 22:28
graphite-app[bot]
graphite-app bot reviewed Aug 1, 2025
View reviewed changes
@overlookmotel overlookmotel mentioned this pull request Aug 1, 2025
Open
@overlookmotel
Copy link
Member Author

@Boshen Playground doesn't seem to have any tests. I assume it's included in CI just to make sure it builds without errors. But have I missed something?

@overlookmotel overlookmotel requested a review from Boshen August 1, 2025 22:49
Boshen
Boshen reviewed Aug 2, 2025
View reviewed changes
.github/workflows/ci.yml
- uses: oxc-project/setup-rust@cd82e1efec7fef815e2c23d296756f31c7cdc03d # v1.0.0
if: steps.filter.outputs.src == 'true'
with:
cache-key: napi
cache-key: napi-${{ matrix.component }}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will bust CI cache https://github.com/oxc-project/oxc/actions/caches, it's already over the limit.

Copy link
Member Author

@overlookmotel overlookmotel Aug 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Weirdly, I'm not sure that's true.

ID         KEY                                                  SIZE        CREATED             ACCESSED            
809899462  v0-rust-napi-Linux-x64-4d15c146-e92cea93             1.01 GiB    about 1 day ago     about 3 minutes ago

814552967  v0-rust-napi-parser-Linux-x64-4d15c146-e92cea93      111.99 MiB  about 12 hours ago  about 7 hours ago
814552798  v0-rust-napi-oxlint2-Linux-x64-4d15c146-e92cea93     203.75 MiB  about 12 hours ago  about 7 hours ago
814552499  v0-rust-napi-e2e-Linux-x64-4d15c146-e92cea93         132.53 MiB  about 12 hours ago  about 7 hours ago
814552909  v0-rust-napi-playground-Linux-x64-4d15c146-e92cea93  195.43 MiB  about 12 hours ago  about 7 hours ago

The individual package caches total 644 MiB, so roughly 400 MiB smaller than the cache from compiling all packages together. Why that would be, I don't know!

(note: napi-e2e cache covers both minify and transform)

Maybe all the caches have accumulated cruft over time, and if we nuke them all and start again, they'll get smaller?

I also noticed something odd. Every cache item has 2 copies e.g.:

cache

Is that normal? Just there's the current cache and the previous most recent?

If so, we're not actually over the limit in practice, because we don't care if the out-of-date caches get deleted.

Copy link
Member Author

@overlookmotel overlookmotel Aug 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To address your comment, I've pushed another commit which returns to building all packages in one job, so there's only 1 cache.

But it doesn't really work - the builds take longer than the tests for all the packages except parser, so to get CI to the finish line faster, building needs to be parallelized, as well as running the tests.

Build job is too slow:

workflow

Copy link
Member Author

@overlookmotel overlookmotel Aug 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've reverted the change which built all packages in one job. So now workflow looks like this:

jobs
  • oxlint2
    • Build: 29s
    • Run tests: 3s
  • parser
    • Build: 31s
    • Clone submodules: 38s
    • Run tests: 30s
  • playground
    • Build: 38s
    • No tests
  • e2e
    • Build oxc-transform: 19s
    • Build oxc-minify: 13s
    • Run transform tests: 5s
    • Run minify tests: 5s
    • Run e2e tests: 51s

4 x build caches instead of 1, but as I said above the total cache size seems to be smaller. And all jobs complete in under 3 mins.

@Boshen I hand this back over to you.

Boshen
Boshen reviewed Aug 2, 2025
View reviewed changes
.github/workflows/ci.yml
component:
- oxlint2
- parser
- playground
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are spawning too many jobs.

Copy link
Member Author

@overlookmotel overlookmotel Aug 2, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The entire purpose of this PR is to spawn more jobs!

Could we request an increase in the concurrent jobs limit?

GitHub Support can increase job concurrency limits for GitHub Actions. To request an increase, submit a support ticket.

https://docs.github.com/en/actions/reference/limits#job-concurrency-limits-for-github-hosted-runners

@Boshen
Copy link
Member

Boshen commented Aug 2, 2025

@Boshen Playground doesn't seem to have any tests. I assume it's included in CI just to make sure it builds without errors. But have I missed something?

To make sure it builds.

@graphite-app graphite-app bot changed the base branch from 08-01-test_napi_parser_disable_raw_transfer_tests_by_default to graphite-base/12743 August 2, 2025 03:04
@graphite-app graphite-app bot force-pushed the 08-01-ci_napi_shard_napi_tests branch from 161af48 to 3a96dc0 Compare August 2, 2025 03:12
@graphite-app graphite-app bot force-pushed the graphite-base/12743 branch from ce53949 to 02504b9 Compare August 2, 2025 03:12
@graphite-app graphite-app bot changed the base branch from graphite-base/12743 to main August 2, 2025 03:13
@graphite-app graphite-app bot force-pushed the 08-01-ci_napi_shard_napi_tests branch from 3a96dc0 to cf62dcd Compare August 2, 2025 03:13
@overlookmotel overlookmotel force-pushed the 08-01-ci_napi_shard_napi_tests branch from cf62dcd to 35e48c6 Compare August 2, 2025 09:22
github-advanced-security[bot]
github-advanced-security bot found potential problems Aug 2, 2025
View reviewed changes
graphite-app[bot]
graphite-app bot reviewed Aug 2, 2025
View reviewed changes
@overlookmotel overlookmotel force-pushed the 08-01-ci_napi_shard_napi_tests branch 3 times, most recently from 1ddff8c to d0517b5 Compare August 2, 2025 10:06
@overlookmotel overlookmotel marked this pull request as draft August 2, 2025 10:14
@overlookmotel overlookmotel mentioned this pull request Aug 2, 2025
Merged
@overlookmotel overlookmotel changed the base branch from main to graphite-base/12743 August 2, 2025 10:26
@overlookmotel overlookmotel force-pushed the 08-01-ci_napi_shard_napi_tests branch from d0517b5 to 509287d Compare August 2, 2025 10:26
@overlookmotel overlookmotel changed the base branch from graphite-base/12743 to 08-02-test_napi_compile_tests_in_debug_mode August 2, 2025 10:26
@overlookmotel overlookmotel changed the base branch from 08-02-test_napi_compile_tests_in_debug_mode to graphite-base/12743 August 2, 2025 11:41
@overlookmotel overlookmotel force-pushed the 08-01-ci_napi_shard_napi_tests branch from 509287d to b02457e Compare August 2, 2025 11:41
@overlookmotel overlookmotel changed the base branch from graphite-base/12743 to 08-02-ci_napi_napi_tests_skip_babel_prettier_submodules August 2, 2025 11:42
@overlookmotel overlookmotel mentioned this pull request Aug 2, 2025
Merged
@graphite-app graphite-app bot changed the base branch from 08-02-ci_napi_napi_tests_skip_babel_prettier_submodules to graphite-base/12743 August 2, 2025 12:19
@graphite-app graphite-app bot force-pushed the graphite-base/12743 branch from 6ada02a to a96e78b Compare August 2, 2025 12:27
@graphite-app graphite-app bot force-pushed the 08-01-ci_napi_shard_napi_tests branch from b02457e to 1ae50b9 Compare August 2, 2025 12:27
@graphite-app graphite-app bot changed the base branch from graphite-base/12743 to main August 2, 2025 12:27
@graphite-app graphite-app bot force-pushed the 08-01-ci_napi_shard_napi_tests branch from 1ae50b9 to bf35776 Compare August 2, 2025 12:27
@overlookmotel overlookmotel force-pushed the 08-01-ci_napi_shard_napi_tests branch from bf35776 to 5d26f1b Compare August 2, 2025 19:56
@Boshen Boshen self-assigned this Aug 11, 2025
@overlookmotel overlookmotel force-pushed the 08-01-ci_napi_shard_napi_tests branch from 5d26f1b to 2b0689d Compare August 11, 2025 09:54
@overlookmotel overlookmotel force-pushed the 08-01-ci_napi_shard_napi_tests branch from 2b0689d to 6a08a8e Compare August 11, 2025 10:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Boshen Boshen Boshen left review comments

@graphite-app graphite-app[bot] graphite-app[bot] left review comments

Copilot code review Copilot Awaiting requested review from Copilot Copilot will automatically review once the pull request is marked ready for review

At least 0 approving reviews are required to merge this pull request.

Assignees

@Boshen Boshen

Labels
A-ast Area - AST
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Speed up NAPI tests in CI with sharding
2 participants
@overlookmotel @Boshen