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
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
9 participants
@addaleax
Member

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 label May 29, 2016

@Trott

This comment has been minimized.

Member

Trott commented May 30, 2016

LGTM if CI doesn't find any surprises

@mscdex mscdex added the crypto label May 30, 2016

@bnoordhuis

This comment has been minimized.

Member

bnoordhuis commented May 30, 2016

Nice, LGTM.

@santigimeno

This comment has been minimized.

Member

santigimeno commented May 30, 2016

LGTM

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

This comment has been minimized.

@mscdex

mscdex May 30, 2016

Contributor

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

This comment has been minimized.

@addaleax

addaleax Jun 5, 2016

Member

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.

if (!common.hasCrypto) {
common.skip('missing crypto');
return;
}

This comment has been minimized.

@mscdex

mscdex May 30, 2016

Contributor

I don't see any crypto/tls/https usage in this test, or is it because npm uses an https-based registry by default?

This comment has been minimized.

@addaleax

addaleax Jun 5, 2016

Member

@mscdex Your assumption is correct; npm uses request which require()s https during its startup phase, it doesn’t even have to talk to the registry for this to fail.

@jasnell

View changes

test/parallel/test-tls-async-cb-after-socket-end.js Outdated
if (!common.hasCrypto) {
common.skip('missing crypto');
return;
}
var path = require('path');
var fs = require('fs');

This comment has been minimized.

@jasnell

jasnell Jun 6, 2016

Member

minor nit: these can be changed to const while you're in here :-)

@jasnell

This comment has been minimized.

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

This comment has been minimized.

Member

jasnell commented Jun 6, 2016

LGTM

@thefourtheye

This comment has been minimized.

Contributor

thefourtheye commented Jun 6, 2016

@jasnell regarding preloading common, #2836 (comment)

test: fix tests for non-crypto builds
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 addaleax:fix-tests-for-disabled-features branch to df2777b Jun 7, 2016

@addaleax

This comment has been minimized.

Member

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

This comment has been minimized.

Member

addaleax commented Jun 8, 2016

Landed in 15cd45c

@addaleax addaleax closed this Jun 8, 2016

@addaleax addaleax deleted the addaleax:fix-tests-for-disabled-features branch Jun 8, 2016

addaleax added a commit that referenced this pull request Jun 8, 2016

test: fix tests for non-crypto builds
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 <rtrott@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@evanlucas

This comment has been minimized.

Member

evanlucas commented Jun 16, 2016

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

@MylesBorins

This comment has been minimized.

Member

MylesBorins commented Jul 11, 2016

@addaleax assuming dont land on v4.x?

@addaleax

This comment has been minimized.

Member

addaleax commented Jul 11, 2016

Seems okay not to backport, yes.

@gibfahn gibfahn referenced this pull request Jun 15, 2017

Closed

Auditing for 6.11.1 #230

2 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment