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 native node test runner, added code coverage support, removed grunt #1604

Merged
merged 4 commits into from Jan 1, 2024

Conversation

hulkish
Copy link
Contributor

@hulkish hulkish commented Dec 24, 2023

This PR does not alter any published code, only modifies devDependencies, tests & dev tooling. In detail:

  • converted tests to use native node test runner
  • removed grunt, mocha, chai & sinon
  • added code coverage support using c8 (since this is not yet supported natively in node)
    • i put line code coverage requirement to 85% - which is the current line coverage of lib/**/*.js.

Some things to note:

  • requires node to be >= 18. Preferably >= 20, whereas the test runner was considered stable.
  • if you decide to run test directly via node --test <test files> (as opposed to npm run test), it's important to note that you will likely need to use --test-concurrency=1 as many of the tests use createServer() from http/https/net and will therefore have dangling handle conflicts if ran concurrently.
  • If you're running.a single test file or multiple files where this is not the case, you can safely exclude --test-concurrency=1
  • In a separate PR, i recommend sticking to a single test file naming convention: 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.

@hulkish hulkish force-pushed the master branch 4 times, most recently from 28e4320 to 7bbcb75 Compare December 24, 2023 03:55
@hulkish
Copy link
Contributor Author

hulkish commented Dec 30, 2023

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

@andris9
Copy link
Member

andris9 commented Dec 30, 2023

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.

@hulkish
Copy link
Contributor Author

hulkish commented Dec 30, 2023

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

@andris9
Copy link
Member

andris9 commented Dec 30, 2023

Did you try to run tests locally under node 18 and 20? It doesn’t work.

@andris9
Copy link
Member

andris9 commented Dec 31, 2023

Btw, if you want to modify the test matrix, you can do so here:

node: [14.x, 16.x, 18.x, 20.x]

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

@hulkish
Copy link
Contributor Author

hulkish commented Dec 31, 2023

Did you try to run tests locally under node 18 and 20? It doesn’t work.

I just ran the tests on node@18.19.0 and node@20.10.0 and everything passed. It shows here in ci that the tests for 18 & 20 were cancelled? Can you try letting them finish running?

@hulkish
Copy link
Contributor Author

hulkish commented Dec 31, 2023

Btw, if you want to modify the test matrix, you can do so here:

node: [14.x, 16.x, 18.x, 20.x]

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

ok I'm going to remove 14.x & 16.x since those won't work with this change.

@hulkish
Copy link
Contributor Author

hulkish commented Dec 31, 2023

Ok, so tests actually passed - but I'll need to lower the minimum coverage required to 85%

@hulkish
Copy link
Contributor Author

hulkish commented Jan 1, 2024

@andris9 says I need to sign my commits, is that really necessary?

@andris9
Copy link
Member

andris9 commented Jan 1, 2024

You don't have to sign these commits even though it would be preferred. Merging now. Thanks.

@andris9 andris9 merged commit be45c1b into nodemailer:master Jan 1, 2024
3 checks passed
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

2 participants