Improve tests execution

[maxpoulin64]

As explained in #259, the current way of running the linters makes it so that if one of them errors out, then the next one is never ran. This causes the situation where you are forced to fix all eslint errors before you can see stylelint errors.

This migrates both to mocha tests, so we now have one single test command to run to deal with everything.

[xPaw]

👍

[xPaw]

Note: Moving HOME here fixes running tests on Windows.

[astorije]

Actually, unless I misunderstood the intent, there is a simpler and more elegant way to do so, with less boilerplate code:

1. Break the npm scripts in multiple scripts:

   "lint:css": "stylelint [...]",
   "lint:js": "eslint [...]",
   "test":  "mocha [...]"

2. Then add a script section to .travis.yml:

   script:
   - npm test
   - npm run lint:js
   - npm run lint:css

And that's it!
All commands in script are run, regardless of the return code of the others:

When one of the build commands returns a non-zero exit code, the Travis CI build runs the subsequent commands as well, and accumulates the build result.

Now, we might still want a single command to run everything with one command, which is something your PR offers. 2 solutions to that:

• Add an extra script for all checks: "all": "npm test && npm run lint:js && npm run lint:css".
• Or rename "test" script into "test:js" or "test:mocha", change the first command to npm run test:(js|mocha) in the .travis.yml file and have the test script run them all: npm run test:(js|mocha) && npm run lint:js && npm run lint:css.

(I prefer the second solution).

Granted, as opposed to your PR, all this makes npm test fail locally as soon as one of the subcommand fails, but checks would be run on the CI regardless of the return code of other checks,. Solving it on the CI is far more important than locally IMO, and the simplicity of this vs. having boilerplate scripts for this justifies the trade-off.

What do you guys think?

[maxpoulin64]

Granted, as opposed to your PR, all this makes npm test fail locally as soon as one of the subcommand fails

Well that's the entire reason why I wanted to make this work in the first place. Sometimes I have multiple things going on at the same time, and a few times I opened a PR thinking my CSS was fine and thought I was all good because I only got errors in unrelated JS code, and then Travis yelled at me because of pages of CSS errors.

[AlMcKinlay]

Would it not be simplest to just change it to this:

"test": "HOME=test/fixtures mocha test/**/*.js; npm run lint",
 "lint": "eslint .; stylelint \"**/*.css\"",

This will both commands whether the first fails or not, and in serial. There is only 1 disadvantage of this, and that's that windows doesn't support ;, but realistically, most people developing on windows are at least using git bash, if not cygwin.

It's much simpler, and makes it much more easy to figure out what's happening. Having mocha actually linting things seems... awkward.

[xPaw]

@YaManicKill using ; does not report correct exit code if one of the two linters fail.

[AlMcKinlay]

True, but that's only an issue for travis, and we can get travis to run the scripts individually, as suggested by @astorije, so that it gets the correct exit codes.

I don't really like this really complex way of dealing with running the linter.

[astorije]

I fully agree with @YaManicKill here. As a matter of fact, I have been using this setup on another project of mine, and using ; has never been an issue locally: see package.json and .travis.yml.
Definitely much simpler than wiring up API calls, lets you locally run tests only or style checker only, and lets Travis CI run and report on all 3 checks independently.
Plus it complies with @maxpoulin64's requirements of having npm test fail locally for one/some of the checks without affecting the others.

@maxpoulin64, are you OK with this solution?

[maxpoulin64]

@astorije Alright, like that for the .travis.yml?

It's not super great (it would be nice to have npm run tests return an appropriate failure number) but it does fix the issue I had and at least it cleans up the tests a little bit.

[astorije]

Is it possible to keep HOME=test/fixtures in the according test to keep this?

[maxpoulin64]

@astorije @YaManicKill All comments should have been addressed now.

[xPaw]

I wonder, perhaps we should be invoking build commands too? They could potentially fail.

[astorije]

They would fail at npm install time. I think this is good as is for now, we can improve later but this PR already solves @maxpoulin64's (and probably others') issue.

[xPaw]

Hmm, it does run it and hides the output. Do we know if travis fails the build if grunt/handlebars build fails at that stage?

[AlMcKinlay]

Alrighty, I'm happy with this now. 👍

Can't see confirmation that @astorije or @xPaw are happy that their comments have been addressed, so will leave the merge till they respond.

[xPaw]

🚢

[xPaw]

Damn. I should have tested the latest version on Windows.

Using ; fails as: missing script: lint:js;, as if it's putting ; as part of the script name...