npm tests poison local cache

[othiym23]

The tests install something in the testing user's local cache. We need to do a pass through the tests and make sure that all are creating, using, and destroying a local cache when running the test suite. All kinds of hilarious garbage is getting left in there.

[chrismeyersfsu]

Below is a listing of files that are modified when running npm test as well as instructions to reproduce the modified file listing.

sudo docker run –user npm -t -i chrismeyers/npm:installed /bin/bash
cd ~/npm
touch time.start
npm test; sudo find / -newer time.start | grep -v -e '^/proc' -e '^/tmp' -e '^/dev' -e '^/sys' | curl -F 'clbin=<-' https://clbin.com

https://clbin.com/y13mg
The password for the user npm in the Docker image is simply npm
This should at least get the ball rolling. The state of the Docker system or the test case need to be modified to be able to apply the same process to the command npm run test-all.

[othiym23]

This is incredibly useful! The next step is to fix the affected tests so that they no longer leave the system in a changed state (as long as git status is the same after npm test, that's good enough).

Thanks, Chris!

[chrismeyersfsu]

By individually invoking each test file and examining the state change I've identified the set of files that cause state change to the cache outside the npm repository.

ls-depth-unmet.js
url-dependencies.js
view.js
install-cli-unicode.js
install-scoped-link.js
install-save-local.js
ls-depth-cli.js
outdated-color.js
publish-config.js
install-at-locally.js
dedupe.js
install-scoped-already-installed.js
install-cli-production.js

There seems to be a deviation in how npm is invoked. I've identified 3 common ways: exec, usage of npm.load and npm.install, and through test/common-tap.js. My solution is to converge the different invocations of npm to use test/common-tap.js. This method causes the test code to touch the most code paths.
Since the code test will be invoking npm via common-tap.js, it makes the most sense to put the cache directive in common-tap.js. We still allow invokes of npm to override the cache directory.
Edit test/common-tap.js exports.npm to default to the below cache directory.

opts.env = opts.env ? opts.env : {};
if (!opts.env.npm_config_cache) {
opts.env.npm_config_cache = __dirname + "/npm_cache";
}

Note that I have successfully implemented this method and changed the first 4 listed test cases to converge. My intention is to continue with this process later tonight. Any feedback that may better this method is welcome.

Edit: Corrected filename

[othiym23]

There seems to be a deviation in how npm is invoked. I've identified 3 common ways: exec, usage of npm.load and npm.install, and through test/common-npm.js. My solution is to converge the different invocations of npm to use test/common-npm.js. This method causes the test code to touch the most code paths.

(You mean ./test/common-tap.js, right?)

This is a solid strategy, and is actually simpler and less labor-intensive than how I would have done it. If you start a PR with what you have so far, I can give you a more detailed review. Early feedback: This is great!

The "npm Way" of cleaning up after these tests is to to write a "zzz-cleanup.js" task that will remove that cache directory after all of the other tap tests have finished, if you want to ensure that the cache directory is cleaned up after every run of "npm test".

[chrismeyersfsu]

#6352 Here is a snapshot of my current changes. Not sure of the state of things. You can get the idea of where I am going with it.

• npm test causes no files to be modified outside of the repo directory
  • resolve 00-verify-ls-ok.js error
  • clean up files after npm test (i.e. test/node_cache/*)
• npm run test-all causes no files to be modified outside of the repo directory

[chrismeyersfsu]

@othiym23 regarding cleanup. Could you elaborate on the "npm Way" ? Is there an example of a cleanup script in the repo?

From my understanding I should make a 99-cleanup-npm-cache.js script.

[othiym23]

1. Whenever possible, each test should clean up after itself, by having a final test() that goes through and removes any files that were created for the purpose of the test (and furthermore, each test should be creating its required fixtures using code, rather than having them stored as static fixtures in the repo, but fixing that is a longer-term project).
2. There should be a final test suite that cleans up everything else that might have been left in a dirty state, as well as doing what it can to clean up from outright crashes of the other test suites. Because this is a lexical sort, it should have a name like zz-cleanup.js – 99-cleanup.js would run before any of the alphabetically-named tests.

[othiym23]

The tap tests are mostly good, but there's still a decent amount of work done to make the legacy tests safer. There's also a good argument to be made that those tests should just be folded into the tap tests (and the disabled tests should also be converted to tap tests or just nuked altogether). Either way, npm is substantially improved by @chrismeyersfsu's work on cleaning up the tap tests.

[npm-robot]

We're closing this issue as it has gone thirty days without activity. In our experience if an issue has gone thirty days without any activity then it's unlikely to be addressed. In the case of bug reports, often the underlying issue will be addressed but finding related issues is quite difficult and often incomplete.

If this was a bug report and it is still relevant then we encourage you to open it again as a new issue. If this was a feature request then you should feel free to open it again, or even better open a PR.

For more information about our new issue aging policies and why we've instituted them please see our blog post.