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: add eslint rule for hasCrypto checking #13813

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
10 participants
@danbev
Member

danbev commented Jun 20, 2017

The motivation for this commit is to pick up early on missing checks for
crypto support (when Node is built --without-ssl).

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

tools

@danbev

This comment has been minimized.

@vsemozhetbyt vsemozhetbyt added the test label Jun 20, 2017

@refack

This comment has been minimized.

Member

refack commented Jun 20, 2017

I love the idea!
How hard will it be to generalize it for inspector as well?

@cjihrig

This comment has been minimized.

Contributor

cjihrig commented Jun 20, 2017

Seems like a good idea to me. Is it possible to tighten up the check though? For example, could it only check IfStatement nodes. In those nodes, check that the condition is !common.hasCrypto or !common.hasFipsCrypto. It could also check that common.skip() is called in the if body.

@cjihrig cjihrig referenced this pull request Jun 20, 2017

Closed

tools: add eslint rule for inspector checking #13814

2 of 2 tasks complete
tools/eslint-rules/crypto-check.js Outdated
// Rule Definition
//------------------------------------------------------------------------------
const msg = 'Please add a hasCrypto check to allow this test to be skippped ' +
'when Node is built \'--without-ssl\'.';

This comment has been minimized.

@gibfahn

gibfahn Jun 20, 2017

Member

Any reason not to:
\' -> "

This comment has been minimized.

@danbev

danbev Jun 20, 2017

Member

No reason, let me fix that too. Thanks!

tools/eslint-rules/crypto-check.js Outdated
//------------------------------------------------------------------------------
// Rule Definition
//------------------------------------------------------------------------------
const msg = 'Please add a hasCrypto check to allow this test to be skippped ' +

This comment has been minimized.

@gibfahn

gibfahn Jun 20, 2017

Member

skippped -> skipped

This comment has been minimized.

@danbev

danbev Jun 20, 2017

Member

Can't believe I missed that, will fix.

@gibfahn

This comment has been minimized.

Member

gibfahn commented Jun 20, 2017

@danbev

This comment has been minimized.

Member

danbev commented Jun 20, 2017

I love the idea!

I can't take credit for the idea as it was @gibfahn's

@danbev

This comment has been minimized.

Member

danbev commented Jun 20, 2017

Seems like a good idea to me. Is it possible to tighten up the check though? For example, could it only check IfStatement nodes. In those nodes, check that the condition is !common.hasCrypto or !common.hasFipsCrypto. It could also check that common.skip() is called in the if body.

I'll take a look, but one issue is that there are places in tests where the check is not used to skip the test but instead only run part of a test, for example in test-async-wrap-getasyncid.js.

@cjihrig

This comment has been minimized.

Contributor

cjihrig commented Jun 20, 2017

If those cases are the exception, we could disable the rule for a line. Or, if it makes sense, refactor the test(s).

tools/eslint-rules/crypto-check.js Outdated
}
}
function testHasCryptoCheck(context, node) {

This comment has been minimized.

@not-an-aardvark

not-an-aardvark Jun 21, 2017

Member

Nitpick: It seems like this function closes over context anyway, so I don't think the context parameter here is needed.

This comment has been minimized.

@danbev

danbev Jun 21, 2017

Member

Good point, I'll fix that. Thanks

@danbev

This comment has been minimized.

Member

danbev commented Jun 21, 2017

If those cases are the exception, we could disable the rule for a line. Or, if it makes sense, refactor the test(s).

I'll take a closer look at the usages of it and see if it makes sense to disable or refactor. Thanks

@danbev

This comment has been minimized.

Member

danbev commented Jun 27, 2017

@Trott Trott referenced this pull request Jun 27, 2017

Closed

tools: update to ESLint 4.1.1 #13946

2 of 2 tasks complete
@danbev

This comment has been minimized.

Member

danbev commented Jun 27, 2017

test/windows-fanned failure looks unrelated

console output:

gyp: c:\workspace\node-test-binary-windows\RUN_SUBSET\2\VS_VERSION\vs2015-x86\label\win2008r2\common.gypi not found (cwd: c:\workspace\node-test-binary-windows\RUN_SUBSET\2\VS_VERSION\vs2015-x86\label\win2008r2\test\addons\06_passing_wrapped_objects_around) while reading includes of binding.gyp while trying to load binding.gyp
gyp ERR! configure error 
gyp ERR! stack Error: `gyp` failed with exit code: 1
gyp ERR! stack     at ChildProcess.onCpExit (c:\workspace\node-test-binary-windows\RUN_SUBSET\2\VS_VERSION\vs2015-x86\label\win2008r2\deps\npm\node_modules\node-gyp\lib\configure.js:336:16)
gyp ERR! stack     at emitTwo (events.js:125:13)
gyp ERR! stack     at ChildProcess.emit (events.js:213:7)
gyp ERR! stack     at Process.ChildProcess._handle.onexit (internal/child_process.js:198:12)
gyp ERR! System Windows_NT 6.1.7601
gyp ERR! command "c:\\workspace\\node-test-binary-windows\\RUN_SUBSET\\2\\VS_VERSION\\vs2015-x86\\label\\win2008r2\\Release\\node.exe" "c:\\workspace\\node-test-binary-windows\\RUN_SUBSET\\2\\VS_VERSION\\vs2015-x86\\label\\win2008r2\\deps\\npm\\node_modules\\node-gyp\\bin\\node-gyp" "rebuild" "--directory=test\\addons\\06_passing_wrapped_objects_around" "--nodedir=c:\\workspace\\node-test-binary-windows\\RUN_SUBSET\\2\\VS_VERSION\\vs2015-x86\\label\\win2008r2"
gyp ERR! cwd c:\workspace\node-test-binary-windows\RUN_SUBSET\2\VS_VERSION\vs2015-x86\label\win2008r2\test\addons\06_passing_wrapped_objects_around
gyp ERR! node -v v9.0.0-pre
gyp ERR! node-gyp -v v3.6.2
gyp ERR! not ok 

c:\workspace\node-test-binary-windows\RUN_SUBSET\2\VS_VERSION\vs2015-x86\label\win2008r2>TASKKILL /F /IM node.exe /T   || TRUE
ERROR: The process "node.exe" not found.

c:\workspace\node-test-binary-windows\RUN_SUBSET\2\VS_VERSION\vs2015-x86\label\win2008r2>TASKKILL /F /IM cctest.exe /T   || TRUE
ERROR: The process "cctest.exe" not found.

c:\workspace\node-test-binary-windows\RUN_SUBSET\2\VS_VERSION\vs2015-x86\label\win2008r2>TASKKILL /F /IM run-tests.exe /T   || TRUE
ERROR: The process "run-tests.exe" not found.

c:\workspace\node-test-binary-windows\RUN_SUBSET\2\VS_VERSION\vs2015-x86\label\win2008r2>TASKKILL /F /IM yes.exe /T   || TRUE
ERROR: The process "yes.exe" not found.

c:\workspace\node-test-binary-windows\RUN_SUBSET\2\VS_VERSION\vs2015-x86\label\win2008r2>rm -rfv test/tmp*   || true

c:\workspace\node-test-binary-windows\RUN_SUBSET\2\VS_VERSION\vs2015-x86\label\win2008r2>rm -vf ../test.tap   || true

c:\workspace\node-test-binary-windows\RUN_SUBSET\2\VS_VERSION\vs2015-x86\label\win2008r2>mv test.tap ../test.tap   || true
mv: cannot stat 'test.tap': No such file or directory

c:\workspace\node-test-binary-windows\RUN_SUBSET\2\VS_VERSION\vs2015-x86\label\win2008r2>git clean -fdx   || true
fatal: Not a git repository (or any of the parent directories): .git

c:\workspace\node-test-binary-windows\RUN_SUBSET\2\VS_VERSION\vs2015-x86\label\win2008r2>git reset --hard   || true
fatal: Not a git repository (or any of the parent directories): .git

c:\workspace\node-test-binary-windows\RUN_SUBSET\2\VS_VERSION\vs2015-x86\label\win2008r2>mv ../test.tap test.tap   || true
mv: cannot stat '../test.tap': No such file or directory

c:\workspace\node-test-binary-windows\RUN_SUBSET\2\VS_VERSION\vs2015-x86\label\win2008r2>exit /b 1 
Build step 'Execute Windows batch command' marked build as failure
TAP Reports Processing: START
Looking for TAP results report in workspace using pattern: test.tap
Did not find any matching files. Setting build result to FAILURE.
Checking ^not ok
Jenkins Text Finder: File set 'test.tap' is empty
Notifying upstream projects of job completion
Finished: FAILURE
@danbev

This comment has been minimized.

Member

danbev commented Jun 27, 2017

@cjihrig I've updated this now with your suggestions. Would you be able to take a look and see if this is what you had in mind?

@cjihrig

This comment has been minimized.

Contributor

cjihrig commented Jun 27, 2017

This is getting closer. Ideally, I was hoping to have a single rule that could be parameterized, and it seems like you're really close there.

I'm also curious why a test like test-process-versions.js needs an eslint comment. That test has a hasCrypto check, but it doesn't require any of the crypto related modules.

@danbev

This comment has been minimized.

Member

danbev commented Jun 28, 2017

I'm also curious why a test like test-process-versions.js needs an eslint comment. That test has a hasCrypto check, but it doesn't require any of the crypto related modules.

Ah good point, let me double check that.

@danbev

This comment has been minimized.

Member

danbev commented Jun 28, 2017

This is getting closer. Ideally, I was hoping to have a single rule that could be parameterized, and it seems like you're really close there.

I'll take another look. I see some similarity between the inspector and crypto rule but not enough to be parameterized fully. For crypto we can check and verify that we are doing it in an if statement and that it contains a skip as you suggested. But for inspector this would simply be a call to common.skipIfInspectorDisabled(). If there is such a call nothing has to be done as it calls skip and exits, but this is not the case for the crypto check.
I might have been staring at this for tool long and therefore not seeing the solution here :)

@refack

This comment has been minimized.

Member

refack commented Jul 2, 2017

After reviewing #14021 I saw there is allot of if (!common.hasCrypto) common.skip('missing crypto'); cruft, so I had a different idea: add logic in common and maybe in test.py that if any of ['crypto', 'tls', 'https'] are required it automagicaly triggers skip

@danbev

This comment has been minimized.

Member

danbev commented Jul 2, 2017

add logic in common and maybe in test.py that if any of ['crypto', 'tls', 'https'] are required it automagicaly triggers skip

There are a few cases where requiring crypto but not skipping the whole test might be appropriate. One example is a test that checks command line options and I think it makes sense for it to skip only the crypto related options and test the others.

@refack

This comment has been minimized.

Member

refack commented Jul 2, 2017

There are a few cases where requiring crypto but not skipping the whole test might be appropriate. One example is a test that checks command line options and I think it makes sense for it to skip only the crypto related options and test the others.

So we could have an equivalent to eslint-disable-line... But anyway my idea gets to the whole explicit vs implicit debate.
Your PR is good, if I figure out an elegant way to follow up on my idea I'll open a new PR later...

@danbev

This comment has been minimized.

Member

danbev commented Jul 6, 2017

@danbev

This comment has been minimized.

Member

danbev commented Jul 6, 2017

test/freebsd failure looks unrelated

console output:

not ok 680 parallel/test-http-server-keep-alive-timeout-slow-client-headers
  ---
  duration_ms: 1.464
  severity: fail
  stack: |-
    events.js:182
          throw er; // Unhandled 'error' event
          ^
    
    Error: This socket has been ended by the other party
        at Socket.writeAfterFIN [as write] (net.js:356:12)
        at Timeout.setTimeout [as _onTimeout] (/usr/home/iojs/build/workspace/node-test-commit-freebsd/nodes/freebsd10-64/test/parallel/test-http-server-keep-alive-timeout-slow-client-headers.js:39:14)
        at ontimeout (timers.js:488:11)
        at tryOnTimeout (timers.js:323:5)
        at Timer.listOnTimeout (timers.js:283:5)

@danbev danbev referenced this pull request Aug 7, 2017

Closed

test: add crypto check to http2 tests #14657

2 of 2 tasks complete
@danbev

This comment has been minimized.

Member

danbev commented Aug 8, 2017

CI: https://ci.nodejs.org/job/node-test-pull-request/9542/ (expecting the linter to fail)

@danbev

This comment has been minimized.

tools: add eslint rule for inspector checking
The motivation for this commit is to pick up early on missing checks for
inspector support (when Node is built --without-inspector).

danbev added some commits Jun 20, 2017

tools: add eslint rule for hasCrypto checking
The motivation for this commit is to pick up early on missing checks for
crypto support (when Node is built --without-ssl).
test: add eslint-disable crypto-check
There are currently usages of common.hasCrypto which are not just for
detecting if crypto support is available and then skip the test in
question. For these case we still want to have a lint error generated
which can then be disabled using an ESLint comment.
@BridgeAR

This comment has been minimized.

Member

BridgeAR commented Aug 30, 2017

Could @jasnell and @not-an-aardvark approve again? I think this could land right after.

@jasnell

This comment has been minimized.

Member

jasnell commented Aug 30, 2017

Still LGTM but a fresh CI before landing would be appreciated

@not-an-aardvark

This comment has been minimized.

Member

not-an-aardvark commented Aug 30, 2017

LGTM

@BridgeAR

This comment has been minimized.

@BridgeAR

This comment has been minimized.

Member

BridgeAR commented Aug 30, 2017

Landed in 50ebac1 and c7dda49

@BridgeAR BridgeAR closed this Aug 30, 2017

BridgeAR added a commit that referenced this pull request Aug 30, 2017

tools: add eslint rule for hasCrypto checking
The motivation for this commit is to pick up early on missing checks for
crypto support (when Node is built --without-ssl).

There are currently usages of common.hasCrypto which are not just for
detecting if crypto support is available and then skip the test in
question. For these case we still want to have a lint error generated
which can then be disabled using an ESLint comment.

PR-URL: #13813
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>

BridgeAR added a commit that referenced this pull request Aug 30, 2017

tools: add eslint rule for inspector checking
The motivation for this commit is to pick up early on missing checks for
inspector support (when Node is built --without-inspector).

PR-URL: #13813
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
@danbev

This comment has been minimized.

Member

danbev commented Aug 31, 2017

Thanks everyone!

addaleax added a commit to addaleax/ayo that referenced this pull request Sep 5, 2017

tools: add eslint rule for hasCrypto checking
The motivation for this commit is to pick up early on missing checks for
crypto support (when Node is built --without-ssl).

There are currently usages of common.hasCrypto which are not just for
detecting if crypto support is available and then skip the test in
question. For these case we still want to have a lint error generated
which can then be disabled using an ESLint comment.

PR-URL: nodejs/node#13813
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>

addaleax added a commit to addaleax/ayo that referenced this pull request Sep 5, 2017

tools: add eslint rule for inspector checking
The motivation for this commit is to pick up early on missing checks for
inspector support (when Node is built --without-inspector).

PR-URL: nodejs/node#13813
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>

MylesBorins added a commit that referenced this pull request Sep 10, 2017

tools: add eslint rule for hasCrypto checking
The motivation for this commit is to pick up early on missing checks for
crypto support (when Node is built --without-ssl).

There are currently usages of common.hasCrypto which are not just for
detecting if crypto support is available and then skip the test in
question. For these case we still want to have a lint error generated
which can then be disabled using an ESLint comment.

PR-URL: #13813
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>

MylesBorins added a commit that referenced this pull request Sep 10, 2017

tools: add eslint rule for inspector checking
The motivation for this commit is to pick up early on missing checks for
inspector support (when Node is built --without-inspector).

PR-URL: #13813
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>

@MylesBorins MylesBorins referenced this pull request Sep 10, 2017

Merged

v8.5.0 proposal #15308

MylesBorins added a commit that referenced this pull request Sep 12, 2017

tools: add eslint rule for hasCrypto checking
The motivation for this commit is to pick up early on missing checks for
crypto support (when Node is built --without-ssl).

There are currently usages of common.hasCrypto which are not just for
detecting if crypto support is available and then skip the test in
question. For these case we still want to have a lint error generated
which can then be disabled using an ESLint comment.

PR-URL: #13813
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>

MylesBorins added a commit that referenced this pull request Sep 12, 2017

tools: add eslint rule for inspector checking
The motivation for this commit is to pick up early on missing checks for
inspector support (when Node is built --without-inspector).

PR-URL: #13813
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Teddy Katz <teddy.katz@gmail.com>
@MylesBorins

This comment has been minimized.

Member

MylesBorins commented Sep 20, 2017

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment