Run lint tests with regular test suite

[joshbruce]

See #1083

Marked version: n/a

Description

PR runs lint tests along with test suite...locally.

Submitter

• Test(s) exist to ensure functionality and minimize regresstion (if no tests added, list tests covering this PR); or,
• no tests required for this PR.
• If submitting new feature, it has been documented in the appropriate places.

Reviewer

• Draft GitHub release notes have been updated.
• case_insensitive_refs is only failing test (remove once CI is in place and all tests pass).
• All lint checks pass (remove once CI is in place).
• Merge PR

[UziTech]

I think these should be separate and only combined in travis. A lot of times I will leave the linting until the end and run the tests frequently when writing code.

[UziTech]

we should just add npm run lint to the README under Running Tests & Contributing

[joshbruce]

Tagging in @davisjam as it was his suggestion - I do think it sounds like a good one. Having to read the README takes me away from the work to learn how to contribute. Having the system tell me, on the other hand...that I can work with quickly.

Sometimes, as a contributor, I'm working on something with a package...the package be broken...I just wanted analyze and fix the issue - maybe not become a full-time contributor and learn all the ins and outs of the documentation. That's my chip into the ring on this one being a worthy feature. First time I saw it was with the USWDS and I really appreciated it - at that time it was the only tests they had in fact.

[Feder1co5oave]

Agree w/ @UziTech

[joshbruce]

Lint doesn't take very long. Maybe break up farther:

test - all the things
test:new
test:original
test:lint - just lint

Then contributors can select a subset of tests to run without the linter, which doesn't take long to run at the moment. Still able to test just the lint rules without fixing them. Covers contributors like @davisjam and myself (without having to read the docs, which no one reads anyway - you know how many issues we had that could have been solved by reading the docs). And, covers Travis testing all the things as well.

[joshbruce]

ps. You know frustrating it is as a contributor when someone tells me to read the docs instead of working collaborative with me, if I wanted to read the docs, I wouldn't be asking the for help. lol

[Feder1co5oave]

That's nonsense to me. Just write to run npm test and npm run lint in the README

[UziTech]

I don't think people really need to know that they need to lint their code as long as it is caught by Travis. For anyone using a modern editor with linting baked in this is a non-issue anyways (let the editor tell them something is wrong)

[davisjam]

as long as it is caught by Travis.

Yup this was really the underlying "problem" in my case. Though if the README had a note about the command for linting that would have been helpful as @Feder1co5oave suggested.

[joshbruce]

Do contributors get to see why Travis failed? (Haven't contributed to a lot of projects that use Travis - usually Circle or nothing.)

Still leaning toward the best of all worlds...big believer in the power of "and". :)

1. The newly minted CONTRIBUTING file in Update docs #1081 has the recommendation of adding the NPM scripts with instructions as well as a mention to both the NPM test and lint scripts in the checklist/environment setup.
2. Travis will tell us now. Having said that, when Travis fails, it fails right now. I didn't even see the failing tests before - just the failing lint. Would prefer, if it's possible, to be able to see all of it. What all failed. Because it's a pretty bad user experience to see something failed in Travis (GitHub) - run back to my local - fix all the things - go back to Travis, which brings up the desired run it locally and have the ability to cherry-pick scope concept.
3. I can run a sub-set while I'm developing. Run a final npm test and be pretty confident that Travis won't say "nope". Submit the PR and, in theory, Travis will say, "yes". No matter what, we're still protected because Travis can say, "nope" and help reinforce the behavior in contributors.

So, docs - check.
Travis - check - with an issue at least for me.
Local - can also be a check all around regarding feedback to users and contributors. (Reminds me of us updating error messages to be more user friendly to improve the developer experience.)

[UziTech]

Clicking on the details link will bring you to the build

then you can click on one of the failing jobs to see the log

[joshbruce]

@UziTech: Right. Sorry, should have been more clear. Someone who's not a collaborator on the repo itself. I know I can see them - by being taken to Travis, but I have write privileges on the repo.

Do I still get that if I don't?

[UziTech]

Ya anyone can see the log for any PR

[UziTech]

Travis also has build stages in beta. I've never set them up but we could look into that.

[UziTech]

[joshbruce]

Tagging #1074 - there's probably other places that idea would be relevant as well.

[Feder1co5oave]

it's a pretty bad user experience to see something failed in Travis (GitHub) - run back to my local - fix all the things - go back to Travis

Totally agree with this.
Just add the two simple commands in the README.
Travis will catch any badly linted code.
Submitters will eventually get bashed in the comments if they don't lint their code and they'll have to go back and fix that.

[davisjam]

Love it!

[joshbruce]

Merging as part of the #1103 work.

[UziTech]

@joshbruce looks like this is what actually broke the node 0.10 test since travis was setup to only lint on other node versions.

I think we should revert this

actually i take that back the only way to get passing tests is to revert this