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: fix tests for non-crypto builds #7056

Closed

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented May 29, 2016

Checklist
  • tests and code linting passes
  • the commit message follows commit guidelines
Affected core subsystem(s)

test

Description of change

Fix running the tests when node was compiled without crypto support. Some of these were introduced in 52bae22, where common was used before it was required.

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

/cc @nodejs/testing

@addaleax addaleax added the test Issues and PRs related to the tests. label May 29, 2016
@Trott
Copy link
Member

Trott commented May 30, 2016

LGTM if CI doesn't find any surprises

@mscdex mscdex added the crypto Issues and PRs related to the crypto subsystem. label May 30, 2016
@bnoordhuis
Copy link
Member

Nice, LGTM.

@santigimeno
Copy link
Member

LGTM

common.skip('missing crypto');
return;
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps the crypto AsyncWrap test can merely be isolated instead of aborting the entire test?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, this test is basically laid out to use all providers, so I guess that would really require its own test file… I’m not sure that’s worth the trouble.

@jasnell
Copy link
Member

jasnell commented Jun 6, 2016

Minor aside: I've been wondering about the possibility of refactoring common.js into a pre-loaded module that makes it (and assert) globals for all tests... allowing us to move away from having to require() it explicitly in every file.

@jasnell
Copy link
Member

jasnell commented Jun 6, 2016

LGTM

@thefourtheye
Copy link
Contributor

@jasnell regarding preloading common, #2836 (comment)

Fix running the tests when node was compiled without crypto
support. Some of these are cleanup after 52bae22, where
common was used before it was required.
@addaleax addaleax force-pushed the fix-tests-for-disabled-features branch from 37f4f9b to df2777b Compare June 7, 2016 05:30
@addaleax
Copy link
Member Author

addaleax commented Jun 7, 2016

Addressed @jasnell’s nit, one more CI: https://ci.nodejs.org/job/node-test-commit/3675/ again because that one had some infrastructure problems: https://ci.nodejs.org/job/node-test-commit/3691/

@addaleax
Copy link
Member Author

addaleax commented Jun 8, 2016

Landed in 15cd45c

@addaleax addaleax closed this Jun 8, 2016
@addaleax addaleax deleted the fix-tests-for-disabled-features branch June 8, 2016 09:43
addaleax added a commit that referenced this pull request Jun 8, 2016
Fix running the tests when node was compiled without crypto
support. Some of these are cleanup after 52bae22, where
common was used before it was required.

PR-URL: #7056
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@evanlucas
Copy link
Contributor

This is not landing cleanly on v6.x, so going to hold off for now

@MylesBorins
Copy link
Contributor

@addaleax assuming dont land on v4.x?

@addaleax
Copy link
Member Author

Seems okay not to backport, yes.

@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Issues and PRs related to the crypto subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants