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

fix: better skip logic #681

Merged
merged 2 commits into from
Nov 30, 2023

Conversation

big-kahuna-burger
Copy link
Contributor

@big-kahuna-burger big-kahuna-burger commented Nov 27, 2023

This adds better logic for skipping certain tests that rely on --no-coverage flag to be provided.
Ideally, all test suites I should be able to run with any of:

  • tap /pattern/
  • tap /pattern/ --no-coverage
  • c8 tap /pattern/

This PR gives me that flexibility for these 2 suites (test/print-*.test.js)

BEFORE:

tap test/print-*.test.js
=>
...
Suites:   ​1 failed​, ​1 passed​, ​2 of 2 completed
Asserts:  ​​​3 failed​, ​25 passed​, ​of 28

c8 tap test/print-*.test.js
=>
... 
Suites:   ​1 failed​, ​1 passed​, ​2 of 2 completed
Asserts:  ​​​3 failed​, ​25 passed​, ​of 28

tap test/print-*.test.js --no-coverage
=> 
...
Suites:   ​2 passed​, ​2 of 2 completed
Asserts:  ​​​28 passed​, ​of 28

NOW:

tap test/print-*.test.js
=> 
...
Suites:   ​2 passed​, ​2 of 2 completed
Asserts:  ​​​20 passed​, ​4 skip​, ​of 24

c8 tap test/print-*.test.js
=> 
...
Suites:   ​2 passed​, ​2 of 2 completed
Asserts:  ​​​20 passed​, ​4 skip​, ​of 24

tap test/print-*.test.js --no-coverage
=> 
...
Suites:   ​2 passed​, ​2 of 2 completed
Asserts:  ​​​28 passed​, ​of 28

Clarification:

  • tap (16.x) without a --no-coverage flag will have a NYC_PROCESS_ID in process.env
  • c8 will assign a coverage/tmp dir into a NODE_V8_COVERAGE env variable

CI was not the reason those tests hanged.
It is c8 in test script that is being run by CI, on top of all unit tests with --no-coverage flag, resulting in effectively undo-ing what that flag was doing for local testing anyway.

Checklist

@big-kahuna-burger
Copy link
Contributor Author

@mcollina PTAL

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@big-kahuna-burger
Copy link
Contributor Author

What is the reason for remaining broken test?
I'm confused as to where tap should be looking for fastify-tsconfig with that setup?

(it's broken locally as well)

* ci: trigger

* ci: trigger

* ci: fail in templates-without-ts-esm now

* ci: fail in template-ts-esm now

* ci: pass now

* ci: now everything works with direct config resolution

* ci: now add back template test and fail

* ci: point to good tsconfig and pass

* fix: nl in config

* ci: rename to unit:ts-esm

* ci: split greedy glob into ts/js, enumerate all template tests separatelly

* ci: all globs are quoted
@big-kahuna-burger
Copy link
Contributor Author

So, those fastify-tsconfig searches errored because of how tsconfig's resolution algorithm works.

Very simple fix is to not point to templates/**/tsconfig.json, and try to resolve fastify-config from there, but to point to a copy of it which I've put in test/configs that has direct resolution to a file in node_modules.

For even more clarity, I've refactored pkg.scripts that deal with tests into the following shape:

test/
├─ unit:cli/
│  ├─ unit:cli-js
│  ├─ unit:cli-ts
├─ unit:templates/
│  ├─ unit:cjs
│  ├─ unit:esm
│  ├─ unit:ts-cjs
│  ├─ unit:ts-esm
├─ test:typescript/
│  ├─ tsd
│  ├─ tsc

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@big-kahuna-burger
Copy link
Contributor Author

big-kahuna-burger commented Nov 29, 2023

@mcollina you can just re-run this failed test.
Happened to hang sometimes in my action as well, likely a timeout issue rather than code correctness issue. Tested locally with node 21 (macos) and it's passing.

@big-kahuna-burger
Copy link
Contributor Author

Looking good now

@mcollina mcollina merged commit 957bd8d into fastify:master Nov 30, 2023
19 checks passed
@big-kahuna-burger big-kahuna-burger deleted the print-tests-context branch November 30, 2023 09:57
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.

2 participants