Skip to content

Applying Linter on Tests #201

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 16 commits into from
Feb 1, 2018
Merged

Applying Linter on Tests #201

merged 16 commits into from
Feb 1, 2018

Conversation

hiranya911
Copy link
Contributor

So far we haven't been linting our test sources. Now that we have upgraded to the latest version of tslint (#194), we should start linting our unit and integration tests. This PR adds the following npm tasks:

npm run lint:src # Lints the source files (everything under src/)
npm run lint:unit # Lints unit tests
npm run lint:integration # Lints integration tests

The aggregate task 'npm run lintruns all of the above, which will run as part ofnpm test` as well (this causes all lint jobs to run with CI).

I also fixed all the existing lint errors in test files. I relaxed the linter rules a little bit for tests by introducing a separate tslint-test.json file.

Copy link
Contributor

@merlinnot merlinnot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you consider adding lint:*:fix too? 🙃

package.json Outdated
@@ -7,12 +7,15 @@
"homepage": "https://firebase.google.com/",
"scripts": {
"build": "gulp build",
"lint": "tslint --project tsconfig-lint.json --format stylish",
"lint": "run-s lint:src lint:unit lint:integration",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

npm-run-all supports glob-like pattern matching, so you can do run-s lint:*.

Why did you decide to run these in sequence instead of parallel?

@hiranya911
Copy link
Contributor Author

hiranya911 commented Feb 1, 2018

@merlinnot What should lint:*:fix do? Run tslint with the --fix flag?

Good idea to run the linter tasks in parallel. I'll make the change.

@merlinnot
Copy link
Contributor

merlinnot commented Feb 1, 2018

Since tslint is a dev dependency (which is a good thing!) one should use node_modules/tslint/bin/... —config ... —fix to fix all auto-fixable issues, which is a bit annoying. It’s much easier to yarn run lint:unit:fix or yarn run lint:fix. You can even leverage existing scripts like so:

{
  "lint:unit:fix": "run-s \"lint:unit —fix\""
}

One day we can even add precommit hooks on top of this, so it fixes everything before committing using husky and lint-staged, but that shouldn’t be a part of this PR.

Copy link
Contributor

@avishalom avishalom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@avishalom avishalom removed their assignment Feb 1, 2018
@hiranya911 hiranya911 merged commit fe3c5db into master Feb 1, 2018
@hiranya911 hiranya911 deleted the hkj-lint-tests branch February 1, 2018 18:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants