Skip to content

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

Merged
ematipico merged 6 commits intomainfrom
chore/use-node-test
Jan 25, 2024
Merged

chore(@astrojs/node): use Node.js for testing#9758
ematipico merged 6 commits intomainfrom
chore/use-node-test

Conversation

@ematipico
Copy link
Member

@ematipico ematipico commented Jan 22, 2024

Changes

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

Testing

Tests should pass

Docs

N/A

@changeset-bot
Copy link

changeset-bot bot commented Jan 22, 2024 •
edited
Loading

⚠️ 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
matthewp
matthewp approved these changes Jan 22, 2024
View reviewed changes
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

bluwy
bluwy reviewed Jan 22, 2024
View reviewed changes
bluwy
bluwy reviewed Jan 22, 2024
View reviewed changes
@Princesseuh
Copy link
Member

Princesseuh commented Jan 22, 2024

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

@ematipico
Copy link
Member Author

ematipico commented Jan 23, 2024

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 ematipico mentioned this pull request Jan 23, 2024
Closed
@ematipico
Copy link
Member Author

ematipico commented Jan 23, 2024 •
edited
Loading

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

@ematipico ematipico closed this Jan 23, 2024
@ematipico ematipico reopened this Jan 23, 2024
@bluwy bluwy mentioned this pull request Jan 25, 2024
Merged
bluwy
bluwy approved these changes Jan 25, 2024
View reviewed changes
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.

@ematipico ematipico force-pushed the chore/use-node-test branch from 4008e33 to 7e0544b Compare January 25, 2024 14:44
@ematipico ematipico force-pushed the chore/use-node-test branch from 7e0544b to bf5669a Compare January 25, 2024 14:48
@ematipico ematipico merged commit fc21a3c into main Jan 25, 2024
@ematipico ematipico deleted the chore/use-node-test branch January 25, 2024 16:17
@ematipico ematipico mentioned this pull request Jan 30, 2024
Closed
19 tasks
@alexanderniebuhr alexanderniebuhr mentioned this pull request Jan 30, 2024
Closed
2 tasks
YoutacRandS-VA
YoutacRandS-VA reviewed Sep 19, 2024
View reviewed changes
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

ematipico added a commit that referenced this pull request Feb 5, 2025
@ematipico @bluwy
chore(@astrojs/node): use Node.js for testing (#9758)
5586469 
* chore(@astrojs/node): use Node.js for testing

* revert file

* address feedback

* feedback

* Run tests in a single process (#9823)

* Run tests in a single process

* Make test less flaky

* chore: remove module

---------

Co-authored-by: Bjorn Lu <[email protected]>
ematipico added a commit that referenced this pull request Feb 5, 2025
@ematipico @bluwy
chore(@astrojs/node): use Node.js for testing (#9758)
4f010d7 
* chore(@astrojs/node): use Node.js for testing

* revert file

* address feedback

* feedback

* Run tests in a single process (#9823)

* Run tests in a single process

* Make test less flaky

* chore: remove module

---------

Co-authored-by: Bjorn Lu <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@matthewp matthewp matthewp approved these changes

@MoustaphaDev MoustaphaDev MoustaphaDev approved these changes

+3 more reviewers

@snowbytes snowbytes snowbytes left review comments

@YoutacRandS-VA YoutacRandS-VA YoutacRandS-VA left review comments

@bluwy bluwy bluwy approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

pkg: integration Related to any renderer integration (scope)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@ematipico @Princesseuh @matthewp @bluwy @MoustaphaDev @snowbytes @YoutacRandS-VA