Add support for testing native modules and Windows

[bzoz]

This PR adds testing of native modules built locally to the smoke tests. It includes all necessary changes to the CITGM and adds modules to package.json:

• bufferutil
• utf-8-validate
• kerberos
• weak
• hiredis
• iconv
• contextify
• thread-sleep
• sqlite3
• v8-profiler
• v8-debuger
• node-sass

It also fixes problem with script key in package.json (details in 4th commit description), adds CITGM itself to smoke tests and makes CITGM ready for use under Windows.

The PR is splitted in 10 commits for easier review.

• First commit adds support for multiple test cases per single module. This is used for example by node-sass and sqlite3 - we test version with prebuild binaries and with ones built locally. With this we can make sure that node works correctly for both the users and module developers. This is the biggest change in this PR.
• Second commit adds displaying of errors form install stage in the default logging level. Without this changes, if npm install failed its console output was lost unless running in verbose.
• Third commit adds support for verifying if npm called node-gyp with either build or rebuild. It is done by setting npm_config_node_gyp (see this npm commit).
• Fourth commit fixes issues with script key in lookup. Right now it is wrongly added to options and not module. Also, script with example-test-script-passing.sh filename is expected to fail by the test. Besides fixing those two errors, this commit makes also test-npm-test.js run under Windows.
• Fifth commit adds support for test-command key in lookup. It works similar to script - a user provided command is used instead of npm test when testing.
• Sixth commit adds install parameter to lookup. This allows for extra parameters to be passed to npm install - e.g. --build-from-source.
• Seventh commit adds node-version to lookup. It can be used to specify minimal node version needed to run given test in citgm-all.
• Eighth commit adds citgm and native modules to lookup.json
• Ninth commit extends node-sass entry in lookup.json, so that both downloaded and build binaries will be tested
• Tenth commit adds support for Windows. It either marks module as flaky or adds workaround for testing with test-command

[gdams]

@bzoz As far as I'm aware, we don't want to add custom scripts to the lookup.json. If a module wants to be tested in CITGM it needs to be able to work with the existing setup. (see #209)

[gibfahn]

@bzoz Is there a reason to do all of this in one PR? A lot of these changes look really useful, but I think it'll be a mammoth effort to get all these commits in at the same time.

I would try to review each commit, but I think it's going to get a bit messy if we have 10 conversations going on at the same time.

[bzoz]

@GeorgeAdams95 this test-command is mostly for Windows support, as a workaround for most of the modules assume Unix shell. It is also used with bcrypt which otherwise will ignore --nodedir parameter passed to CITGM. Another module is kerberos for which we want to only check compilation – testing requires specific environment to be set, so we want to skip that. Both of those things can be done in another way, but with ‘test-command` citgm-all can run most of the module tests on Windows out of the box.

@gibfahn this PR makes sense as a whole – it adds new modules to lookup.json and all required functionality. Besides first commit the changes are rather small and straightforward. I’ve split it into 10 commits for easier review. But if you really feel that it would be beneficial I can go ahead and split this PR.

[gibfahn]

@bzoz I (personally) think splitting up the PR makes sense, not least because I think it'll make a big difference to how quickly things get merged.

For example I think commit 2 is a small and useful change which should be easy to review/approve/merge, whereas commit 8 is adding a bunch of modules, which might need more time to be discussed (and have more requested changes).

I think @thealphanerd is quite keen on people raising issues to discuss new features, so we can discuss before actually implementing. Obviously no need to raise them now 😄

Having a test-command for windows is really something that should be supported by the modules themselves if possible. If we have to maintain custom test commands then we run the risk of being out of date.

What is the benefit for Node core in testing a module (e.g. node-sass) with the downloaded binaries? I see why you might want it for module authors, but how much benefit do you get using citgm vs. an npm install --build-from-source node-sass && npm test?

What are the reasons for adding those modules? I think going forwards the aim is to have a clear justification for each module added (link).

Instead of node-version, I think it would be better to extend the existing skip option to handle the same range of options that flaky does.

[MylesBorins]

@bzoz thanks for all the hard work you have put in here.

So I'm going to agree with the sentiment in this thread that there are just too many moving parts going on here.

Each of the individual features are going to need their own PR so that we can assess them and land them on their own merit. They are also going to need to come with tests, which it does not appear there are at the moment. The entire repo has around 95% code coverage. The tool is currently broken due to a bug in the reporter stack, but I should hopefully get something in this week that can repair that. I change as big as this should not lower the overall coverage of the repo.

I understand that some of these are building on each other, and it is frustrating to have to move through this slower process. Hopefully you can understand why we need to approach each of these features individually

/cc @jasnell for his opinion

[bzoz]

Sure, I see your point. I will split this PR in separate PRs that I will open later today.