discuss: Should we alter the behavior of make test?

[Trott]

The behavior was changed and then reverted after objections. I take this to mean that the initial discussion didn't include enough folks.

This is an attempt to document why we might want to make certain changes, and what those options are.

Option 1: Status Quo (Change Nothing)

Pros

• Battle-tested. If it mostly works, why fix it?
• Doesn't move anyone's cheese. People are used to it, let's not mess up their current workflows.

Cons

• Linting runs fast. Tests are slow. With the current set up, you can run tests and wait many minutes before finding out that your tests pass but you have linting errors. At this point, you fix the linting errors, but then you have to run all the tests again. Hope you wanted to go get a second cup of coffee while the tests run a second time.
• If a test fails, you won't be told about linting errors, even if those linting errors might point you at precisely the problem. ("Oh, I didn't mean to have that case block fall through to the next one! Thanks for pointing it out, ESLint!")

Option 2: Lint Before Testing

Pros

• Linting runs fast. So tell me the linting errors now rather than making me wait through the long test run before bothering to lint.

Cons

• This will mess with some people's current workflow and that can be really annoying, especially if you've used the same workflow for years.

Option 3: Keep Testing Before Linting, But Lint Even If Tests Fail

Pros

• If your linting errors are related to the cause of your test failures, it's nice to be told about them.
• If you have a locally flaky test, you still get lint results, rather than only finding out about your lint errors in CI. (Sure, you can run the lint job via make or directly, but we've seen that people don't.)

Cons

• Code to do this in Makefile will be ugly. I suspect it will be portable, but I don't know that and it may be annoying to test across all relevant platforms in advance
• Output may be confusing. If there was a test error or a jslint error, but cpplint reports "No errors found!" then this may confuse newcomers. This is likely fixable, but there are general possible issues around changing output from the job that may only come to light as we use it.
• You still have to wait through the tests to get your lint results.

Refs: #5602
Refs: #5470

/cc @Fishrock123 @bnoordhuis @thealphanerd @evanlucas @jasnell @jbergstroem

[Trott]

Other options that haven't been discussed yet or have only been mentioned in passing, but since they might come up:

• Create a separate job that flips the order so people who want to run linting first every time can just use that job.
• Pull linting out of make test entirely. Let there be a make test and a make lint.
• Anything else I've missed?

[evanlucas]

what if tools.test.py was the one that was responsible for running eslint and could disabled via a flag?

[jasnell]

could we not use an environment variable for this? For those who want linting first, set the env var, otherwise it stays as it is currently.

[jbergstroem]

If we're doing a vote I'd like to promote "splitting up test and lint" into an option I can +1. That'd be my preferred solution.

[Fishrock123]

I'd be more willing to have some slightly complex make code rather than have new contributors PR twice. Could we maybe get a reviewable PR of that if anyone has the time? (I may try if no-one else does..)

[Trott]

@Fishrock123 You mean for Option 3 above? If so, here you go: #5609

[jbergstroem]

#5618 would likely cause more issues since it is discussing the option of removing linting libraries from release tarballs. It's necessarily not a vote on disconnecting lint and test but the rules will likely grow complex with the logic we're introducing as a result of above options.

[Trott]

Closing, I think we've settled on "No change".