feat: add linting job to CI workflow and simplify test script #158
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ljharb
requested changes
Mar 23, 2025
| "pretest": "[ \"$NODE_LTS_LATEST\" != \"\" ] && [ \"$MATRIX_NODE_VERSION\" != \"$NODE_LTS_LATEST\" ] && echo 'Skipping linting' || npm run lint", | ||
| "test": "npm run tests-only", | ||
| "tests-only": "tap", | ||
| "test": "tap", |
There was a problem hiding this comment.
npm test should always run everything. you run npm run tests-only if you only want to run the tests.
Comment on lines
+12
to
+26
| lint: | ||
| name: Lint | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v4 | ||
| - name: Setup Node.js | ||
| uses: actions/setup-node@v4 | ||
| with: | ||
| node-version: 'lts/*' | ||
|
|
||
| - name: Install dependencies | ||
| run: npm install --ignore-scripts --include=dev | ||
|
|
||
| - name: Run lint | ||
| run: npm run lint |
There was a problem hiding this comment.
i believe we already run the linter via pretest
There was a problem hiding this comment.
So, should the linter run after the tests using posttest? I've never agreed that the linter should run with the tests—it should be something separate and in a different job.
There was a problem hiding this comment.
Locally, everything should run with npm test.
The CI jobs should be configured so that the linter runs once, and the tests run on every supported node version (minor or major).
Thus, npm test runs everything, and CI workflows run more granular scripts.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The linter is not the tests. I want to run the tests peacefully without worrying about the linter. I can worry about the linter later after finishing my changes.