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

test: skip doctool tests when js-yaml is missing #7218

Merged
merged 1 commit into from
Jul 6, 2016

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Jun 8, 2016

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

test

Description of change

Skip the doctool tests when js-yaml, which is currently require()d from the eslint source tree, is missing. This can happen, for example, because eslint is not included in the release source tarballs.

Fixes: #7201
Ref (introduced in): #6495

CI: https://ci.nodejs.org/job/node-test-commit/3692/

@addaleax addaleax added doc Issues and PRs related to the documentations. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. labels Jun 8, 2016
@silverwind
Copy link
Contributor

LGTM, but we should eventually find a solution. What's js-yaml used for, by the way? Just the "added" comments?

@addaleax
Copy link
Member Author

addaleax commented Jun 8, 2016

+1 – it’s one of the things that have led me to open #6999. And yeah, exactly that… I don’t know if it would have been my first choice but I carried it over from the original #3867 because I didn’t see much speaking against it.

@Fishrock123
Copy link
Contributor

Hmmm, when will this be skipped? Only from the tarball, not on the CI?

@addaleax
Copy link
Member Author

@Fishrock123 yes, only the tarball.

There’s also #7290 that addresses the same problem by actually adding the dependencies, and I don’t have any too strong feelings, which is why I didn’t merge this yet.

@Fishrock123
Copy link
Contributor

I personally prefer this approach.

@addaleax
Copy link
Member Author

addaleax commented Jul 5, 2016

@thefourtheye Would you mind if I landed this? That doesn’t need to be the end of discussion, but it might be nice to have make test from tarballs working again.

@thefourtheye
Copy link
Contributor

No problem. Working tarball is important. LGTM.

Skip the doctool tests when js-yaml, which is currently `require()`d
from the eslint source tree, is missing. This can happen, for example,
because eslint is not included in the release source tarballs.

Fixes: nodejs#7201
Ref: nodejs#6495
PR-URL: nodejs#7218
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
@addaleax addaleax force-pushed the skip-doctool-tests-without-eslint branch from 77f3034 to af273b5 Compare July 6, 2016 10:41
@addaleax
Copy link
Member Author

addaleax commented Jul 6, 2016

rebased, CI again just to be sure: https://ci.nodejs.org/job/node-test-commit/3991/

@addaleax addaleax merged commit af273b5 into nodejs:master Jul 6, 2016
@addaleax addaleax deleted the skip-doctool-tests-without-eslint branch July 6, 2016 12:27
@MylesBorins
Copy link
Contributor

@addaleax lts? If so can you backport? Not landing cleanly

(thanks for being so responsive today!)

evanlucas pushed a commit that referenced this pull request Jul 12, 2016
Skip the doctool tests when js-yaml, which is currently `require()`d
from the eslint source tree, is missing. This can happen, for example,
because eslint is not included in the release source tarballs.

Fixes: #7201
Ref: #6495
PR-URL: #7218
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
addaleax added a commit to addaleax/node that referenced this pull request Jul 12, 2016
Skip the doctool tests when js-yaml, which is currently `require()`d
from the eslint source tree, is missing. This can happen, for example,
because eslint is not included in the release source tarballs.

Fixes: nodejs#7201
Ref: nodejs#6495
PR-URL: nodejs#7218
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
Skip the doctool tests when js-yaml, which is currently `require()`d
from the eslint source tree, is missing. This can happen, for example,
because eslint is not included in the release source tarballs.

Fixes: #7201
Ref: #6495
PR-URL: #7218
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 12, 2016
Skip the doctool tests when js-yaml, which is currently `require()`d
from the eslint source tree, is missing. This can happen, for example,
because eslint is not included in the release source tarballs.

Fixes: #7201
Ref: #6495
PR-URL: #7218
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jul 12, 2016
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
Skip the doctool tests when js-yaml, which is currently `require()`d
from the eslint source tree, is missing. This can happen, for example,
because eslint is not included in the release source tarballs.

Fixes: #7201
Ref: #6495
PR-URL: #7218
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 14, 2016
Skip the doctool tests when js-yaml, which is currently `require()`d
from the eslint source tree, is missing. This can happen, for example,
because eslint is not included in the release source tarballs.

Fixes: #7201
Ref: #6495
PR-URL: #7218
Reviewed-By: Roman Reiss <[email protected]>
Reviewed-By: Sakthipriyan Vairamani <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. 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