Page MenuHomePhabricator

Make npm test faster
Closed, DeclinedPublic

Description

Because of multiple npm run invocations, npm test is quite slower as it should be. See

invocationrealusersys
current npm test0m1.661s0m1.517s0m0.219s
raw cli calls0m0.799s0m0.758s0m0.100s
marvin (master=) => time npm test

> marvin@0.0.0 pretest /Users/jhernandez/dev/marvin
> npm run lint:all


> marvin@0.0.0 lint:all /Users/jhernandez/dev/marvin
> npm run lint -- '{src,test}/**/*.js'


> marvin@0.0.0 lint /Users/jhernandez/dev/marvin
> eslint --cache --max-warnings 0 "{src,test}/**/*.js"


> marvin@0.0.0 test /Users/jhernandez/dev/marvin
> mocha '{src,test}/**/*.test.js'



  page()
    ✓ incorporeal


  1 passing (9ms)


real	0m1.661s
user	0m1.517s
sys	0m0.219s
marvin (master=) => time sh -c "eslint --cache --max-warnings 0  '{src,test}/**/*.js' && mocha '{src,test}/**/*.test.js'"


  page()
    ✓ incorporeal


  1 passing (8ms)


real	0m0.799s
user	0m0.758s
sys	0m0.100s

This command is going to be run a lot in dev machines, precommit hook and CI so it should be faster.

Event Timeline

We can move these commands to a makefile or shell script if you like.

@Niedzielski do you see the same slowdowns? My computer is particularly shitty

Yes, I see about 2.5s when executing from NPM. 1.25s when calling the commands directly.

That is consistently double the time 😕

Change 370944 had a related patch set uploaded (by Niedzielski; owner: Sniedzielski):
[marvin@master] Chore: improve test performance

https://gerrit.wikimedia.org/r/370944

I have a proposed patch up here. When evaluating, please do time ./marvin test. The NPM package.json test script is just for CI. This patch appears to solve the problem on my machine but adds a layer of indirection you might hate! :] It's fine to abandon and try another option (or just accept that our current implementation has maximum intuitiveness but is just slightly slow)!

Some results:

invocationrealusersys
current npm test0m1.921s0m1.718s0m0.257s
patch npm test0m1.245s0m1.135s0m0.173s
patch shell script0m0.966s0m0.883s0m0.127s

Even without the nested npm runs, just one npm causes some overhead.

Thanks for the patch, it is a good POC, but I'm not convinced as we lose some of the simplicity :( It is also shell dependent which means if we want to stay cross-env it is going to be harder than using npm scripts (windows wise mainly).

I think we should stall this effort until it becomes a bigger problem. Thanks for trying. If you think we should pursue this further now let me know and we can chat about our options.

Change 370944 abandoned by Niedzielski:
Chore: improve test performance

https://gerrit.wikimedia.org/r/370944