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

tests: tools/node_modules is not present in tarball #51145

Closed
AdamMajer opened this issue Dec 13, 2023 · 5 comments
Closed

tests: tools/node_modules is not present in tarball #51145

AdamMajer opened this issue Dec 13, 2023 · 5 comments

Comments

@AdamMajer
Copy link
Contributor

Version

No response

Platform

No response

Subsystem

No response

What steps will reproduce the bug?

No response

How often does it reproduce? Is there a required condition?

No response

What is the expected behavior? Why is that the expected behavior?

No response

What do you see instead?

In git repository there is

https://github.com/nodejs/node/tree/main/tools/node_modules

in the source tarball, there is nothing there. This breaks at least

https://github.com/nodejs/node/blob/main/benchmark/misc/startup-cli-version.js#L13

Additional information

No response

@AdamMajer
Copy link
Contributor Author

Seems this is kind of test regression introduced in #50684 where this tests relies on this removed tool. tools/node_modules are removed from the tarball for some time, meaning many years.

AdamMajer added a commit to AdamMajer/node that referenced this issue Dec 13, 2023
tools/node_modules is removed from the tarball so it should not be used
as part of unit tests or benchmarks.

Fixes: nodejs#51145
AdamMajer added a commit to AdamMajer/node that referenced this issue Dec 13, 2023
tools/node_modules is removed from the tarball so it should not be used
as part of unit tests or benchmarks.

Fixes: nodejs#51145
Refs: nodejs#50684
@richardlau
Copy link
Member

Ideally this should have been caught by the https://github.com/nodejs/node/blob/main/.github/workflows/build-tarball.yml workflow in #50684 but it was not 😞.

@AdamMajer
Copy link
Contributor Author

      - name: Copy directories needed for testing
        run: |
          cp -r tools/node_modules $TAR_DIR/tools
          cp -r tools/eslint-rules $TAR_DIR/tools

this seems to copy things over 🤔 I think either these should be then shipped or should not be needed for tests?

@targos
Copy link
Member

targos commented Dec 14, 2023

I'm not sure it's reasonable to expect that benchmarks and their tests work with the release tarball.

@AdamMajer
Copy link
Contributor Author

Well, it's been working since at least nodejs 4.x 😉 I'm happy if tools/node_modules is removed in tarball, but there is no package-lock.json that allows it to be installed again. This removed part of tarball is about 22MB of disk space.

The tools/eslint-rules are only 100kB so this can be added to the tarball.

AdamMajer added a commit to AdamMajer/node that referenced this issue Dec 15, 2023
tools/node_modules is removed from the tarball so it should not be used
as part of unit tests or benchmarks.

Fixes: nodejs#51145
Refs: nodejs#50684
RafaelGSS pushed a commit that referenced this issue Jan 2, 2024
tools/node_modules is removed from the tarball so it should not be used
as part of unit tests or benchmarks.

Fixes: #51145
Refs: #50684
PR-URL: #51146
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
richardlau pushed a commit that referenced this issue Mar 25, 2024
tools/node_modules is removed from the tarball so it should not be used
as part of unit tests or benchmarks.

Fixes: #51145
Refs: #50684
PR-URL: #51146
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants