mock s3 v2

[bmacnaughton]

introduction

like the previous stab at this, the env var node_pre_gyp_mock_s3 controls whether s3 mocking is used. if set to a non-empty value s3 will be mocked. this version doesn't change any package.json nor run.util.js at the cost of embedding some additional code in lib/node-pre-gyp.js.

specifics

there are two components:

• lib/util/s3_setup.js provides the s3 storage interface to other modules in the program. if node_pre_gyp_mock_s3 is empty or not present then AWS.S3 is used as the s3 provider. if it is not empty then mock-aws-s3 is used as the s3 provider.
• lib/node-pre-gyp.js now has code that uses nock to mock the http interface to s3 when s3 is being mocked. this interface is specific to the bucket being used for testing and it is hardcoded.

if node_pre_gyp_mock_s3 is set then the program generates an os-specific temporary directory name for the mock s3 storage.

misc

• minor updates to .gitignore and .travis.yml
• there is a little code for debug logging in test/run.util.js
• node_pre_gyp_mock_s3 is set to true in .travis.yml and appveyor.yaml. not sure what setting should be the default.
• probably should close previous PR

[bmacnaughton]

both tests suites are failing on redirects. it's failing locally too, so i'll take a look at it tomorrow or monday.

[bmacnaughton]

finally. it took me a bit to understand how to use nock effectively.

i think if you can run tests with and without mocking that the coverage will be good. i believe the slight decrease in coverage is because i added a fair amount of code to s3_setup.js but only the mocking paths are executed.

[springmeyer]

This is amazing work! Thank you.

[springmeyer]

This is great. Even better I think would be another matrix/job so that:

• We test both mocking and no mocking
• Only the mocking job is used with coverage is tested and uploaded

Sound good? If so, could you add another job that uses node_pre_gyp_mock_s3=false? You should be able to do this by moving the env into each matrix item.

[bmacnaughton]

it's easy enough to move the env into the matrix and add the false env setting.

but i'm not sure what you mean by "only the mocking job is with coverage is tested and uploaded".

and maybe it's related, but if mocking is false won't all external PR tests always fail?

[springmeyer]

I've been thinking we should move to npm ci, which I think does this. Do you know if there is a functional or important difference between npm ci and npm install --package-lock-only?

[bmacnaughton]

actually, the command that is executed further down is npm ci and that fails because package.json and package-lock.json are out of sync. at least that's what i was seeing. that's why only this test was failing - it's the only one that did npm install request

[springmeyer]

Could you add a little more detail on why this is needed? My question is coming from the deeper question of whether there is any way we could avoid having this code in lib/. If not I don't think this is a blocker, but want to understand more why it is needed.

[bmacnaughton]

my original goal (in the original PR) was to not modify any of the code in lib (or as little as possible). but to do so meant modifying the script command in package.json which led to more problems (different env var settings for windows/linux, changes in what program was actually run, etc. - that stuff is still in the other PR if you want to compare).

i originally put this here because i am not sure whether nock patches the http/https modules after they have been loaded or only when they are loaded. if only when they are loaded, i wanted to load nock as early as possible. i did not get around to testing that so i don't know.

but one way or the other the code has to go into lib/. the reason is that it is lib/install that needs to use the mocked https endpoint, not the test module. while test/fetch.test.js requires lib/install.js and invokes the function directly, the test/build.test.js tests execute bin/node-pre-gyp via a subprocess and there is no good way that i came up with (see original PR for not-so-good way) to have the mock code in the same node instance in that case. my original approach was to prefix the commands with node -r mocking-module <regular command> but that had the knock-on consequences of fiddling with the commands in order to get them to work.

[bmacnaughton]

and, if not in lib/node-pre-gyp.js it would have to go into lib/install.js which seemed worse.

a couple options:

i can make the code slightly less invasive by not requiring that it be turned on.

i could also just let the code check the env var again. i didn't because i was trying to localize knowledge of the env var to the single s3_setup module. if i do that it can look like:

require('./util/s3_setup').get_mockS3Http();

and if you want to avoid the 'mock' reference it could just be changed to get_s3Http().

[bmacnaughton]

@springmeyer - any thoughts on this? i am working on putting together publishing for our addon that requires what is in master now. if there is no release i can tie our install to a specific commit or possibly a .tgz file until there is a release. but i'd also like to resolve these outstanding questions as well.

let me know how i can help.

[springmeyer]

Thanks @bmacnaughton - I'd hoped to have time to review again to be able to suggest some concrete adjustments to travis yaml. But I don't want to delay you and my next weeks are going to be busy. So, to avoid blocking you I'll merge now and I might find time to touch up the PR later. Thanks!

[bmacnaughton]

that's awesome! thanks. are you going to get a release to npm out based on master? maybe an rc? and i'm happy to discuss/implement the alternatives i suggested.

[springmeyer]

I would recommend pinning your work to a specific master gitsha for now. If that does not work for some reason bug me next week to create an rc. Or bug me for commit access so you can do it yourself :)