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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use npm-run-all to run test and lint scripts #1604

Merged
merged 8 commits into from Nov 12, 2021

Conversation

bmish
Copy link
Sponsor Collaborator

@bmish bmish commented Nov 11, 2021

  • Reorganizes test and lint NPM scripts with popular tool npm-run-all
  • Adds convenient scripts for running different types of linting or tests
  • Runs linting in parallel but keeps output of linters separate
  • Does not run linting and testing in parallel, since this would break the real-time/live output from ava
  • Shows complete output from all types of linting and tests, even if one type fails (previously, the test script would exit early if one type failed, so you wouldn't be able to see all the failures)
  • Avoids using && which might not have as good of cross-platform support according to the npm-run-all readme

package.json Outdated Show resolved Hide resolved
Copy link
Collaborator

@fisker fisker left a comment

Choose a reason for hiding this comment

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

I suggest also add a fix version.

package.json Outdated
"test": "xo && nyc ava && markdownlint '**/*.md'",
"lint": "npm-run-all --continue-on-error --aggregate-output --parallel lint:*",
"lint:js": "xo",
"lint:md": "markdownlint '**/*.md'",
Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe only double quote is supported on Windows.

Copy link
Sponsor Collaborator Author

Choose a reason for hiding this comment

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

Fixed.

@bmish
Copy link
Sponsor Collaborator Author

bmish commented Nov 12, 2021

I suggest also add a fix version.

Added :fix scripts.

@fisker
Copy link
Collaborator

fisker commented Nov 12, 2021

Sorry, I didn't make myself clear.

I prefer

{
	"lint": "",
	"lint:a": "",
	"lint:b": "",
	"fix": "",
	"fix:a": "",
	"fix:b": ""
}

yarn fix and yarn fix:js is simpler than yarn lint:fix and yarn lint:js:fix

WDYT?

Here is how I use.

@bmish
Copy link
Sponsor Collaborator Author

bmish commented Nov 12, 2021

@fisker that's fine with me, updated.

Copy link
Collaborator

@fisker fisker left a comment

Choose a reason for hiding this comment

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

@bmish
Copy link
Sponsor Collaborator Author

bmish commented Nov 12, 2021

Can you also update the action config file? main/.github/workflows/main.yml#L28

Updated to have CI run npm run lint and npm run test:js.

However, this is why I personally prefer to have the test and lint scripts completely independent, so that CI can just run the lint and test scripts separately.

@fisker
Copy link
Collaborator

fisker commented Nov 12, 2021

Me too. But I guess sindresorhus prefers it in one.

@bmish
Copy link
Sponsor Collaborator Author

bmish commented Nov 12, 2021

It looks like Windows CI doesn't like something about running nyc ava even though the tests appear to have passed.

npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! eslint-plugin-unicorn@38.0.1 test:js: `nyc ava`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the eslint-plugin-unicorn@38.0.1 test:js script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

@fisker
Copy link
Collaborator

fisker commented Nov 12, 2021

Try switch nyc to c8?

@bmish
Copy link
Sponsor Collaborator Author

bmish commented Nov 12, 2021

Try switch nyc to c8?

I doubt nyc itself is the problem as I've used that in other repos with Windows CI.

@fisker
Copy link
Collaborator

fisker commented Nov 12, 2021

I ran into similar thing, and fixed by using c8. But I'm not sure, we can figure later.

@sindresorhus sindresorhus merged commit 70ae05a into sindresorhus:main Nov 12, 2021
@fisker fisker mentioned this pull request Nov 17, 2021
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.

None yet

3 participants