Framework: Enable Parallel Tests on Circle-CI #3793
Conversation
blowery
added
Framework
[Status] Needs Review
|
Nice, gotta love a |
blowery
changed the title
| - client/**/*.jsx | ||
| - server/**/*.js | ||
| - server/**/*.jsx | ||
| - NODE_ENV=development make build && NODE_ENV=development ./bin/run-tests: |
There was a problem hiding this comment.
In run-tests we overwrite NODE_ENV. Looking at this line it seems like the intention is to use development env. If that's true, then probably we can also skip creating new test.json file. It could be that I missed something of course, I have never worked with Circle CI before :)
There was a problem hiding this comment.
This actually gets really confusing. Some Makefile ignore an already set NODE_ENV and reset it to TEST. And run-tests sets it, but doesn't export it or pass it along, so subprocs don't seem to pick it up.
In the current system I think we're actually using the outer node_env just to generate the config file? Would be nice to not have the switcheroo and just use an env of test everywhere.
There was a problem hiding this comment.
Yup, complete agreement. I was debating doing that here, but copying the dev config was simpler and less prone to introduce crazy things.
|
|
|
|
||
| test: | ||
| override: | ||
| - ./node_modules/.bin/eslint --quiet: |
There was a problem hiding this comment.
Do we worry about the maintainability of splitting this off of the lint Makefile target?
(First time I've noticed mixedindentlint as a dependent target of lint)
There was a problem hiding this comment.
There's no way to pass args to a makefile target (that i'm aware of?), so ... not sure how else to do it. maybe move to a npm run lint which runs eslint and move the makefile to use that?
There was a problem hiding this comment.
maybe move to a
npm run lintwhich runs eslint and move the makefile to use that?
I'm a bit of the opinion this is the direction we should head anyways 😄 (Related: #3520)
Is npm any better about passing arguments to the scripted commands? In my own experiences, I seem to recall this being an issue.
There was a problem hiding this comment.
No idea. Can find out.
There was a problem hiding this comment.
There was a problem hiding this comment.
In ac95127
One odd bit: passing $(JS_FILES) through made the linter bomb. Unsure why.
|
OK, now |
|
Confirmed I'm personally still confused on a few bits, specifically around when I'll leave it to you to decide whether there's concerns there. Otherwise, this is looking good to go 👍 |
I think now that we have a test.json we can just use NODE_ENV=test and be fine. It didn't work in the past because there was no test config to pull from. How it was working for the server tests is still a bit of a mystery to me. |
blowery
force-pushed
the
try/parallel-tests-on-circle
branch
from
01b76bb
to
19deaa6
Compare
|
OK, happy now. Squashing and landing. |
Also enable caching in eslint, rework how we run the linter to make it work from an npm script, and add a test config file.
blowery
force-pushed
the
try/parallel-tests-on-circle
branch
from
19deaa6
to
42ebab3
Compare
Framework: Enable Parallel Tests on Circle-CI
nylen
removed
the
[Status] Needs Review
Part of #3822.
This is an attempt to enable test and lint splitting on CircleCI, to take advantage of parallel test containers.
To really make this work, we need to drop the test configuration variable in the web settings, as that is running in addition to the split tests.
Early returns show that moving to 3 containers would drop overall test time to around 4m. Going to our full 8 containers should drop it to around 2m, even before all the test refactoring.
One problem with going full 8 is that the desktop app shares our container set and it seems to be constantly rebuilding itself, even with no changes to the code.