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

chore: improve coverage and good practices for tests #2472

Merged
merged 15 commits into from
Mar 6, 2024

Conversation

wellwelwel
Copy link
Collaborator

@wellwelwel wellwelwel commented Mar 6, 2024

Refactoring Every Test 🔬

CI

  • Update or upgrade all CI Action versions, one by one
  • Cache from Linux, OSX and Website are now individuals
  • Windows cache now works
  • Include Bun latest and migrate ci-bun.yml to ci-linux.yml
  • The tests are now more conservative:
    • It test all versions from MySQL (including the latest) in a specific CI (ci-mysql.yml)
    • It test all Node.js versions from 14 (he's back) to 21
    • Now, every test in all CI is tested in both SSL 0|1 and Compression 0|1
  • The coverage now uses only c8: c8 ... npm run test to generate the coverage data
    • There's no more "if builtin-runner", all tests now work together 🐷✨
    • The coverage test now has its own CI
  • Now both tests and typings are checked for Lint rules (both Prettier and ESLint)

Tests

  • Since we now use Poku, it doesn't matter if the tests are ESM or CJS, so we can run all the tests at once (and yes, it will work for all Node.js versions - and Bun) 🐷
  • Now all tests are reorganized as .test.
  • The builtin-runner has been moved to esm
  • Now Bun also uses the same paths as Node.js, but filtering (for now) the basic select tests
  • To maintain good practices, as we use .esm in test/esm, all tests in test/unit and test/integration are now .cjs
  • Indirectly, using .test. should to remove the false positives warnings from CodeQL in tests (I guess) 🧑🏻‍🔧
  • All tests that were using utest have been migrated to ESM
  • Now there's a test to ensure the extensions in test paths

Dependencies


Some problems were found 🐞

  • All the tests that were using utest weren't being processed:
    • While the test was waiting for a callback, the process was completed as "successfully" before
    • By turn these tests back, an Issue was found for namedPlaceholders

@wellwelwel wellwelwel added refactor bun security dependencies Pull requests that update a dependency file node.js labels Mar 6, 2024
@wellwelwel wellwelwel merged commit 7ae15a5 into sidorares:master Mar 6, 2024
64 checks passed
@wellwelwel
Copy link
Collaborator Author

wellwelwel commented Mar 6, 2024

@sidorares, if do you like, we can upload the coverage results to Codecov ☔️

  • It will cover all PRs and compare the actual coverage from master branch (for all contributors)
  • We can use the badge in README

To do this, you'll need to create a secret called CODECOV_TOKEN and allow the Codecov App to this repository.

If you do, send me a ping and I'll adapt the coverage workflow 🙋🏻‍♂️


An example:

Screenshot 2024-03-06 at 00 14 36

@sidorares
Copy link
Owner

To do this, you'll need to create a secret called CODECOV_TOKEN and allow the Codecov App to this repository.

sure, I'm ok to use codecov, I'll add the secret

do you know if codecov can merge multiple coverage results or do we need to do that ourselves? Different matrix runs skip some parts ( platform of feature flags specific ) so Ideally we want to consider the "union" of all matrix run coverages

@wellwelwel
Copy link
Collaborator Author

wellwelwel commented Mar 6, 2024

so Ideally we want to consider the "union" of all matrix run coverages

That's why I migrated the coverage CI to its own workflow, which is happening now 🙋🏻‍♂️

I don't know if it's clear, but using Poku, it runs all the tests (CJS and ESM) in a unique flow, so there's no need to merge multiple coverage results, since there is no longer distinction between the previous built-in:

  • All tests run on all CI in all versions and platforms, without exception.

@wellwelwel
Copy link
Collaborator Author

do you know if codecov can merge multiple coverage results or do we need to do that ourselves?

Looking at the Codecov documentation, it says that Codecov will search for all "coverage" files available.

Alternatively, we can specify the files manually.

In my previous comment, I forgot to consider the specified flags.

@sidorares
Copy link
Owner

@wellwelwel CODECOV_TOKEN secret added

@wellwelwel
Copy link
Collaborator Author

wellwelwel commented Mar 12, 2024

CODECOV_TOKEN secret added

@sidorares, all done ☔️


And...

By using Poku to fully test MySQL2, in addition to the prestige of it, it's an even greater incentive to continue investing on it. So, thank you again 💙

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bun dependencies Pull requests that update a dependency file node.js refactor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants