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

tools: fix doctool tests in release builds #7290

Closed
wants to merge 1 commit into from

Conversation

thefourtheye
Copy link
Contributor

Checklist
  • make -j4 test (UNIX) or vcbuild test nosign (Windows) passes
  • the commit message follows commit guidelines
Affected core subsystem(s)

tools, test

Description of change

In the release builds, the eslint directory under tools directory is
not included. But, the hack introduced in
#6495 depends on the js-yaml
dependency of eslint. So, the make test fails with the release source
tarball.

This patch introduces js-yaml and all its dependencies.

cc @addaleax @nodejs/build @nodejs/release

In the release builds, the eslint directory under tools directory is
not included. But, the hack introduced in
nodejs#6495 depends on the js-yaml
dependency of eslint. So, the `make test` fails with the release source
tarball.

This patch introduces `js-yaml` and all its dependencies.
@thefourtheye thefourtheye added test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. labels Jun 13, 2016
@addaleax
Copy link
Member

I think one reason that eslint is not included in the release tarballs is exactly that its dependencies add some weight to it, which is why I proposed #7218 the way I did. I haven’t done any measurements myself, though.

@thefourtheye
Copy link
Contributor Author

Ya, I understand that. This could be an alternative, if we decide to run doctool tests in release source tarballs, as well.

@addaleax
Copy link
Member

Not against that! :) If we do it this way, we might also consider including the linter in the tarball, no?

@thefourtheye
Copy link
Contributor Author

thefourtheye commented Jun 13, 2016

We lint the code and build it. That may not fail depends on the environment. But tests might fail in different environments, right? Thats why I thought doctool tests should be allowed to run properly.

@addaleax
Copy link
Member

addaleax commented Jun 13, 2016

¯_(ツ)_/¯ was just thinking about the tarball size, but yeah, you have a point.

@MylesBorins
Copy link
Contributor

@thefourtheye do you know what the difference in size there is for the tarball with these deps included?

Do you think we are getting close to where the doctool + it's tests should perhaps be in another repo (similar to readable-stream)? I know that having it in the repo is a great integration test... but perhaps we are going in the wrong direction if we are increasing the tarball size in order to run the tests

@jbergstroem
Copy link
Member

We (ok, I) removed eslint for tarball size alone (motivation being that it was a developer tool and developers should use git instead), so incrementally adding it again it probably worse than just reintroducing it. See #5695.

@thefourtheye
Copy link
Contributor Author

@jbergstroem We are not reintroducing eslint in the tarball. The doctool tests depend on one of the dependencies of eslint. When js-yaml module is required, it will actually require the eslint/node_modules/js-yaml. Since we remove eslint from the tarball, doctool tests get their own copy of js-yaml with this patch.

@jbergstroem
Copy link
Member

@thefourtheye yeah, I was just suggesting that we could reintroduce eslint if we actually need it (or parts of it). When I suggested it to go it was because there wasn't a need for it. Obviously functionality precedes any tarball savings.

@Fishrock123
Copy link
Contributor

Prefer #7218 imo

@thefourtheye thefourtheye deleted the fix-release-test branch July 6, 2016 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants