Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Track the requirements #9

Open
2 tasks done
Eomm opened this issue Jan 24, 2022 · 10 comments
Open
2 tasks done

Track the requirements #9

Eomm opened this issue Jan 24, 2022 · 10 comments

Comments

@Eomm
Copy link
Member

Eomm commented Jan 24, 2022

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure the feature has not already been requested

馃殌 Feature Proposal

After #8 we should document in a more clear way the requirements for the project to adopt the reusable workflow.

For example:

  • npm test should not run the linter
  • the lint script should run all the linters

Motivation

I think in future the requirements will grow up, so we should start listing it before it will be too late to understand how to set up a repository

Example

No response

@darkgl0w
Copy link
Member

Totally agree with you.

I have a couple of "standard" npm scripts I use across my projects that improve the overall DX:

"ci:test": "npm run coverage && npm run test:types",
"coverage": "npm run unit -- --coverage-report=lcovonly",
"lint": "standard",
"lint:fix": "standard --fix",
"test": "npm run lint && npm run unit && npm run test:types",
"test:types": "tsd"
"unit": "tap -J \"test/**/*.test.js\"",
"unit:errors": "npm run unit -- -R terse",
"unit:report": "npm run unit -- --coverage-report=html",
"unit:verbose": "npm run unit -- -Rspec"

@Fdawgs
Copy link
Member

Fdawgs commented Jan 24, 2022

Just to add, this was partially discussed in the fastify/plugins Discussions last year.

Throwing all of the tests into the test script would be ideal (as @darkgl0w's example shows), as this the standard name for test scripts, with a separate lint script for... linting.

@darkgl0w
Copy link
Member

darkgl0w commented Jan 24, 2022

@Fdawgs something like this then ?

"coverage": "npm run unit -- --coverage-report=lcovonly", // useful to generate coverage report consumed by code coverage services
"lint": "standard",
"lint:fix": "standard --fix",
"test": "npm run coverage && npm run test:types", // target: ci run
"test:dev": "npm run lint && npm run unit && npm run test:types", // target: pre-commit run on user workflow (e.g.: my use case)
"test:types": "tsd"
"unit": "tap -J \"test/**/*.test.js\"", // just run the test suite
"unit:errors": "npm run unit -- -R terse", // run the test suite and display only errors (thanks to James Sumners that's now one of my best friends 馃槢)
"unit:report": "npm run unit -- --coverage-report=html", // useful to track the code coverage
"unit:verbose": "npm run unit -- -Rspec" // useful to track down tests failures and maintain a good DX (test descriptions, operations, plans ... etc)

@climba03003
Copy link
Member

Just to add, this was partially discussed in the fastify/plugins Discussions last year.

Throwing all of the tests into the test script would be ideal (as @darkgl0w's example shows), as this the standard name for test scripts, with a separate lint script for... linting.

test script is used for pre-commit hooks. For test only, it should go for unit.

@darkgl0w
Copy link
Member

test script is used for pre-commit hooks. For test only, it should go for unit.

This is how I use it on my workflow, I split each operations on dedicated scripts and in addition I have a CI dedicated script.

@darkgl0w
Copy link
Member

update
we can solve this by allowing the customization of the script names to run (fallback to test).

https://github.com/darkgl0w/fastify-swagger/blob/exp-alt/.github/workflows/ci.yml
custom test script name CI run

@Fdawgs
Copy link
Member

Fdawgs commented Feb 15, 2022

@fastify/plugins, if we can agree on what name the CI test script should have, as discussed in comments above, then this can get moving and can be applied to more plugin repos.

@darkgl0w
Copy link
Member

darkgl0w commented Feb 15, 2022

In order of preference I would go for test:ci, ci:test or just ci so there will be no ambiguity about what is the script target.

PS: It would be awesome if we could agree on removing (at least) node 10 from the test matrix too.

@jsumners
Copy link
Member

test:ci would be the correct name. And it should include -R terse.

@Eomm
Copy link
Member Author

Eomm commented Feb 16, 2022

I think there are two topics:

  1. the scripts needed for the workflow
  2. the scripts needed by the developer

The first one requires:

  • lint: to run once the lining as optimization
  • test: run the test (usually lint+test+types). It is optimized for automation and reporting

I think we should not use test:ci because we are not talking about a complex application, but a simple node.js module. So, adding a dedicated script may lead to running out of sync the test and test:ci commands.

The latter is for us: it helps us to remember simple commands:

  • lint:fix
  • test:unit: runs just the test
  • test:types

The other stuff adds too much noise into the scripts tag in my opinion.

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

No branches or pull requests

5 participants