-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 native node test runner, added code coverage support, removed grunt #1604
Conversation
28e4320
to
7bbcb75
Compare
Hmm, looks like failing because glob patterns for tests are only available in node 21+, I guess I could remove the quotes to rely on shell glob expansion |
Thank you for the effort but these tests do not seem to pass. npm test should work at least both with node 18 and 20. Currently, it partially works with 21 only. |
hmm, looks like the jobs for 18 & 20 were cancelled... I updated the scripts to use shell globs - looks like the tests would have passed if not cancelled |
Did you try to run tests locally under node 18 and 20? It doesn’t work. |
Btw, if you want to modify the test matrix, you can do so here: nodemailer/.github/workflows/test.yml Line 11 in 4233f6f
In general, it seems a great idea to move from Grunt to node native test runner but the tests must pass on max supported platforms. In this case on Node 18, 20, 21 |
I just ran the tests on |
… test runner, added c8 for coverage
ok I'm going to remove 14.x & 16.x since those won't work with this change. |
Ok, so tests actually passed - but I'll need to lower the minimum coverage required to 85% |
@andris9 says I need to sign my commits, is that really necessary? |
You don't have to sign these commits even though it would be preferred. Merging now. Thanks. |
This PR does not alter any published code, only modifies
devDependencies
, tests & dev tooling. In detail:c8
(since this is not yet supported natively in node)lib/**/*.js
.Some things to note:
node
to be >= 18. Preferably >= 20, whereas the test runner was considered stable.node --test <test files>
(as opposed tonpm run test
), it's important to note that you will likely need to use--test-concurrency=1
as many of the tests usecreateServer()
fromhttp
/https
/net
and will therefore have dangling handle conflicts if ran concurrently.--test-concurrency=1
foo.test.js
. Currently this repo seems to use 2 different naming conventions for test files:foo-test.js
&foo.test.js
for seemingly no reason. For brevity, I chose not to make this change in this PR.