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.

Sign up for GitHub

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

Jump to bottom

Fix tests in Node.js v16 #4597

Merged
merged 8 commits into from
Feb 26, 2023
Merged

Fix tests in Node.js v16 #4597

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
24ce60d
use node 18 for package manager ci
mcollina Feb 24, 2023
2af74dc
fixup
mcollina Feb 24, 2023
aba99cc
fixup
mcollina Feb 24, 2023
2521397
fixup
mcollina Feb 24, 2023
ade3e74
restore v14 and v16 in package manager ci
mcollina Feb 24, 2023
b4e8d84
fixup
mcollina Feb 24, 2023
ca4f400
fixup
mcollina Feb 26, 2023
3a835d6
fixup
mcollina Feb 26, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/package-manager-ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ jobs:
strategy:
matrix:
# Maintenance and active LTS
node-version: [14, 16]
node-version: [18]
os: [ubuntu-latest]

steps:
Expand Down Expand Up @@ -43,7 +43,7 @@ jobs:
strategy:
matrix:
# Maintenance and active LTS
node-version: [14, 16]
node-version: [18]
os: [ubuntu-latest]

steps:
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
"lint:typescript": "eslint -c types/.eslintrc.json types/**/*.d.ts test/types/**/*.test-d.ts",
"prepublishOnly": "PREPUBLISH=true tap --no-check-coverage test/build/**.test.js",
"test": "npm run lint && npm run unit && npm run test:typescript",
"test:ci": "npm run unit -- -R terse --cov --coverage-report=lcovonly && npm run test:typescript",
"test:ci": "npm run unit -- --cov --coverage-report=lcovonly && npm run test:typescript",
Copy link
Member

Choose a reason for hiding this comment

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

Why are we removing -R terse?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess we can add it back. I couldn't understand what the problem was without the full tap output.

Copy link
Member

Choose a reason for hiding this comment

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

I'd be in favour of more output in CI so folks don't need to add this to debug

Copy link
Member

Choose a reason for hiding this comment

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

Except 99% of the time we only care about what test has failed and finding it in among the thousands of lines of "good" output is basically impossible.

Copy link
Member

Choose a reason for hiding this comment

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

I get that, though, why do you only care about what test has failed? Just knowing what test failed is sometimes enough information to move forward, but you also often have to know how the test failed or the more details. If CI just tells you which test failed, then you need to rerun locally, wasting time because CI already had the information, it just didn't show it to you. I don't feel super strongly, but I feel like CMD+Fing for the failure and having all the information at hand is kinda nice, instead of making humans toil more to reproduce that information.

Something we've done at my company is log the verbose output to a file and have that uploaded as a test artifact as well.

Copy link
Member

Choose a reason for hiding this comment

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

Try out -R terse with a failing test.

"test:report": "npm run lint && npm run unit:report && npm run test:typescript",
"test:typescript": "tsc test/types/import.ts && tsd",
"test:watch": "npm run unit -- -w --no-coverage-report -R terse",
Expand Down