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: add crypto's setEngine test #10786

Closed

Conversation

hiroppy
Copy link
Member

@hiroppy hiroppy commented Jan 13, 2017

Add tests for crypto.setEngine.
https://github.com/nodejs/node/blob/master/lib/crypto.js#L621

There is not a normal test because I did not have an idea that how to designate the engine(path or id) in the test :'-(
error:25066067:DSO support routines:DLFCN_LOAD:could not load the shared library

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@nodejs-github-bot nodejs-github-bot added test Issues and PRs related to the tests. lts-watch-v6.x labels Jan 13, 2017
@mscdex mscdex added the crypto Issues and PRs related to the crypto subsystem. label Jan 13, 2017
@jasnell
Copy link
Member

jasnell commented Jan 13, 2017

This is a difficult one to test because it's entirely dependent on a number of factors that are not under our control (such as what other engines may be available on the OS being tested). It's definitely something that we should try to get fixed and tested tho so I really appreciate the effort here.

@jasnell
Copy link
Member

jasnell commented Jan 13, 2017

@nodejs/crypto

@hiroppy
Copy link
Member Author

hiroppy commented Jan 13, 2017

Thank you for the answer. So leave only the exception tests.(verification tests for id and flags)

@mhdawson
Copy link
Member

mhdawson commented Jan 13, 2017

It also shows up as missing coverage in the code coverage numbers:https://coverage.nodejs.org/coverage-57f6a106fbc69a47/root/crypto.js.html ==> line 621

So good to add a test.

Ideally we would have some sort of dummy engine that we could actually set, but it seems like a good start to at least test the failure cases.

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM, provided CI passes.

@jasnell
Copy link
Member

jasnell commented Jan 15, 2017

@jasnell
Copy link
Member

jasnell commented Jan 16, 2017

Failures in CI appear to be unrelated but just in case: https://ci.nodejs.org/job/node-test-pull-request/5888/

@shigeki
Copy link
Contributor

shigeki commented Jan 17, 2017

I think we can make tests for both static and dynamic engines by adding new crypto.getEngineList() API. I can submit a PR soon. How about it?

@jasnell
Copy link
Member

jasnell commented Jan 17, 2017

That would be worthwhile, I think.
I was also wondering about the feasibility of having a non-op engine in our tests that could be used but I've never implemented an openssl crypto engine so have no idea how feasible that is ;-)

@shigeki
Copy link
Contributor

shigeki commented Jan 17, 2017

Here is WIP of my PR in 0312f084d43e220c0448f80c6144a3d1f8877426 for crypto.getEngineList().
We can check static engine by looking for the engine id of dynamic that is included by default.
I also add a node_test_engine module in the test fixture to check dynamic loading. But gyp is very platform dependent. I have to check them all on CI.

@jasnell
Copy link
Member

jasnell commented Mar 24, 2017

Updates on this one?

@jasnell jasnell added the stalled Issues and PRs that are stalled. label Mar 24, 2017
@shigeki
Copy link
Contributor

shigeki commented Mar 25, 2017

This was already fixed in #11143. Closing.

@shigeki shigeki closed this Mar 25, 2017
@hiroppy hiroppy deleted the feature/add-test-crypto-set-engine.js branch March 25, 2017 00:30
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. stalled Issues and PRs that are stalled. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants