Skip to content
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

chore(@astrojs/node): use Node.js for testing #9758

Merged
merged 6 commits into from
Jan 25, 2024
Merged

Conversation

ematipico
Copy link
Member

Changes

This PR moves @astrojs/node to use Node.js for testing

Testing

Tests should pass

Docs

N/A

Copy link

changeset-bot bot commented Jan 22, 2024

⚠️ No Changeset found

Latest commit: bf5669a

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions github-actions bot added pkg: integration Related to any renderer integration (scope) pkg: astro Related to the core `astro` package (scope) and removed pkg: astro Related to the core `astro` package (scope) labels Jan 22, 2024
Copy link
Contributor

@matthewp matthewp left a comment

Choose a reason for hiding this comment

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

its-happening.gif

@Princesseuh
Copy link
Member

Would love to see if there's any differences in speed!

@ematipico
Copy link
Member Author

This PR

Benchmark 1: pnpm test
  Time (mean ± σ):     17.103 s ±  0.678 s    [User: 26.434 s, System: 4.905 s]
  Range (min … max):   16.390 s … 18.307 s    10 runs

main

Benchmark 1: pnpm test
  Time (mean ± σ):      5.786 s ±  0.232 s    [User: 7.962 s, System: 1.420 s]
  Range (min … max):    5.520 s …  6.272 s    10 runs

@ematipico
Copy link
Member Author

ematipico commented Jan 23, 2024

I am closing this for now because the regressions are concerning. Node.js runs tests in parallel, spawning a process for each test. While this decision is understandable, there should be a way to allow to run tests in one single process.

I opened a feature request here: nodejs/node#51548

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

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

LGTM after fixing the conflicts and the small nit below.

packages/integrations/node/test/api-route.test.js Outdated Show resolved Hide resolved
@ematipico ematipico merged commit fc21a3c into main Jan 25, 2024
13 checks passed
@ematipico ematipico deleted the chore/use-node-test branch January 25, 2024 16:17
Copy link

@YoutacRandS-VA YoutacRandS-VA left a comment

Choose a reason for hiding this comment

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

Apply

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: integration Related to any renderer integration (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants