feat: check that there are actually modules in node_modules

[jehy]

• Ready for review
• Follows CONTRIBUTING rules
• Reviewed by Snyk internal team

What does this PR do?

This checks that there are actually some modules in node_modules directory.
I also added a ToDo because it is better to check that all deps from package.json are installed.

Where should the reviewer start?

From the beginning, of cause.

How should this be manually tested?

There can be three cases:

1. node_modules exist and there is something - should not change (test it)
2. node_modules exist but is empty - should return new error message
3. node_modules does not exist - should not change (old error message)

Any background context you want to provide?

This will break testing projects which have zero dependencies and have an empty node_modules directory but I don't think that this testing makes any sense - it is clearly a false positive.

What are the relevant tickets?

https:/snyk/snyk/issues/126

Screenshots

Additional questions

[CLAassistant]

All committers have signed the CLA.

[jehy]

Travis CI seems broken - it fails because snyk is not authorized.

[adrukh]

@jehy thanks for this contribution! Overall a very clean implementation!

I'm slightly concerned with this flow:

2. node_modules exist but is empty - should return new error message

When using npm@3 on a project with no dependencies, npm install creates an empty node_modules folder. I'd like snyk test to successfully test 0 dependencies on this project. Your suggestion will make this case result in a failure, and be a breaking change for our users.

Can you suggest a way to overcome this? I'd attempt to run npm -v and check the version, acting accordingly to the result of this run.

It would also be great to see tests added for this scenario - see https:/snyk/snyk/blob/master/test/missing-node-modules.test.js#L11 for a relevant test.

Thanks for your effort, it is much welcome! I'd like to help seeing this through :)

[jehy]

'd like snyk test to successfully test 0 dependencies on this project. Your suggestion will make this case result in a failure, and be a breaking change for our users.

I don't think that it's right to test project with zero dependencies, but I understand that breaking changes are pretty bad.

In https:/snyk/snyk/issues/126 I described why relying on npm version is unreliable. But I added another commit to this pull request - it checks whether all dependencies from package.json exist in node_modules. That should be fine and projects will zero dependencies should pass. Also it will throw error if not all modules are currently installed.

[jehy]

On January 25, I added this PR.
It was not reviewed up to September, when finally merge conflict rose.

It think that snyk developers should close pull requests section on github if they are are not going to work with them.

[lili2311]

Closing this as the issue is resolved as per confirmation on the issue: https:/snyk/snyk/issues/126