nodejs / node-core-utils

CLI tools for Node.js Core collaborators
https://nodejs.github.io/node-core-utils/
MIT License
234 stars 106 forks source link

chore(test): migrate to `node:test` #670

Closed aduh95 closed 1 year ago

targos commented 1 year ago

Segmentation fault (core dumped)

Interesting.

aduh95 commented 1 year ago

Probably related to https://github.com/nodejs/node/pull/46674

MoLow commented 1 year ago

Probably related to https://github.com/nodejs/node/pull/46674

I am pretty sure it is since node 16 is lacking many features of the test runner, the mentioned PR simply adds the coverage to the output, it does not solve a bug

MoLow commented 1 year ago

I was able to run on all three node versions in my branch: https://github.com/aduh95/node-core-utils/compare/node--test...MoLow:node-core-utils:node--test the main 2 issues that are not supported on node 16 are https://github.com/nodejs/node/pull/45055 https://github.com/nodejs/node/pull/45161

MoLow commented 1 year ago

@aduh95 the tests pass but coverage can only be collected on node >= 18

MoLow commented 1 year ago

I think it might be a fair mitigation to run tests without coverage on node 16

aduh95 commented 1 year ago

It looks like there are a few test failures on v18.x and v19.x though

MoLow commented 1 year ago

yeah since node --test now runs files inside test/fixtures, I think running test-unit would have the same outcome as running test-all had before, unless I am missing something

MoLow commented 1 year ago

@aduh95 this should work: https://github.com/aduh95/node-core-utils/commit/d7ea450b4f5612d5508920aee953960602481977

codecov[bot] commented 1 year ago

Codecov Report

Patch coverage has no change and project coverage change: +0.04 :tada:

Comparison is base (1ad864e) 83.40% compared to head (b6e8de8) 83.45%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #670 +/- ## ========================================== + Coverage 83.40% 83.45% +0.04% ========================================== Files 37 37 Lines 4146 4146 ========================================== + Hits 3458 3460 +2 + Misses 688 686 -2 ``` [see 1 file with indirect coverage changes](https://codecov.io/gh/nodejs/node-core-utils/pull/670/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None) Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=None)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

aduh95 commented 1 year ago

For some reason, CI picks up v18.15.0 only on Windows 🤔

MoLow commented 1 year ago

@aduh95 seems like actions/setup-node@v3 should have check-latest: true

MoLow commented 1 year ago

@aduh95 this requires amending the commit message(s)

aduh95 commented 1 year ago

No it doesn't, the title of the PR (which is what GitHub uses as the default commit message) is complying with Conventional Commit spec.

MoLow commented 1 year ago

so this check should be fixed to only look at the PR name? https://github.com/nodejs/node-core-utils/actions/runs/4430791079/jobs/7772963001?pr=670

aduh95 commented 1 year ago

The issue is that some PR are being "Rebase and merge"d, in which case it makes sense to check every commits.