Investigate switching to jest #430
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
manabshy
added
do not merge
Closed
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
https://eaflood.atlassian.net/browse/WATER-4144 For those not aware, the dev team are now doing all new work in water-abstraction-system. This is how we are side-stepping the issues of working with the legacy code base and moving towards a simpler, better future! When putting together the project we selected Hapi Lab for the unit test framework. The main framework for all Defra Node-based projects is Hapi. So, early Defra projects also selected Lab for unit testing. It was also the framework for Charging Module API so the dev team were familiar with it. But it has limitations. Other Node unit test frameworks include more functionality that we have to add to Lab ourselves. Documentation as well as online examples are limited. Plus we’ve been keeping an eye on other Defra projects. Nowadays Jest is the framework of choice. All the Future Farming and Rod Licencing repos use it plus anything new we’ve seen appear on GitHub. It probably helps that Jest is the most popular unit test framework for Node. Certainly, most contract devs who join the team are likely to have experience with it. We’d be surprised if they’d even heard of Lab! So, before we get too far down the path of our bright and shiny future now is the time to look at what it would take to switch to Jest.
We have been trying to make VSCode more Jest 'whizzy' hence playing with `jsconfig.json`. Also some of us like a tool called wallaby. These are personal preferences and their config should not pollute the source code. So, adding them to gitignore to prevent this happening in future.
We believe we should be able to cover all our mocking requirements with what Jest brings to the table. We also have concerns about this package which has not had a release in 4 years and many outstanding issues and PR's.
Everything is global with Jest which standardjs is getting in a tizz about. This tweak to package.json will stop errors appear in linting.
The `--silent` false just means `console.log()` output will now appear which helps when debugging unit tests. Also on our journey we saw ``` Jest did not exit one second after the test run has completed. This usually means that there are asynchronous operations that weren't stopped in your tests. Consider running Jest with `--detectOpenHandles` to troubleshoot this issue. ``` So, we're doing as it suggests for now in case we see any others!
When trying to work with the DB in the tests we were getting ``` Jest did not exit one second after the test run has completed. This usually means that there are asynchronous operations that weren't stopped in your tests. Consider running Jest with `--detectOpenHandles` to troubleshoot this issue. ``` Googling led us to [Having trouble running Jest, unhandled Promises](Vincit/objection.js#1015) and [Need to explicit destroy knex in order to stop the script](Vincit/objection.js#534). The second issue included a comment that setting the pool min size to 0 and idleTimeoutMillis to 500 would solve the problem. We tried it and it worked!
We want to retain the ability to run tests that interact with the DB. But we are also concious that tests are now run asynchronously. So, we can't keep wiping the DB at the start of every test. Instead, we add this global setup file that will be called before Jest runs any tests and it ensure the test DB is clean. We'll then need to take this into account when migrating tests from Lab to Jest.
We wanted to be able to run jest without lots of errors caused by the un-migrated tests. So, specifying what tests to match was the first change we made. From there we solved our DB clean problem by referencing a new global jest setup file. Code coverage is critical for us migrating so that config has been added. What we have at this time ensures we when running tests can see the gaps whilst still providing the lcov file that SonarCloud needs. Finally, we write our tests with the aim that they read as documentation. The best way to see this is when running the tests. The default output removed that ability but adding `verbose: true` means our test output now reads like documentation again.
To support migrating to tests that don't clean the DB and can work together we need to start generating values randomly rather than hard coding them. Often things like references involve number sequences so the first key function we need is something that will generate a random number.
We relied on a lot of hard coded values in our test helpers. To allow us to run DB tests asynchronously we need to avoid trying to insert multiple records with the same values. This change goes through all our helpers and switches out hard coded ID's for generated ones. Where relevant we also now randomly generate the references used in various records.
Cruikshanks
added a commit
that referenced
this pull request
Oct 5, 2023
https://eaflood.atlassian.net/browse/WATER-4144 We've been investigating changing our unit test framework recently (see [Investigate switching to jest](#430) and [water-abstraction-ava](https://github.com/DEFRA/water-abstraction-ava)). A key issue we'll have if we switch is that most other frameworks run tests asynchronously. This means our pattern of clean-setup-assert won't work. At best we can clean before any tests run but then the test data we setup must not clash. To achieve that we need to make the test data the helpers generate as unique as possible, which means moving away from hard coded values. We're still playing with frameworks but there is nothing stopping us making this change now ahead of a decision. So, this change covers making the helpers generate more unique and fixing any tests that breaks.
Everything is global with Jest which standardjs is getting in a tizz about. This tweak to package.json will stop errors appear in linting.
The `--silent` false just means `console.log()` output will now appear which helps when debugging unit tests. Also on our journey we saw ``` Jest did not exit one second after the test run has completed. This usually means that there are asynchronous operations that weren't stopped in your tests. Consider running Jest with `--detectOpenHandles` to troubleshoot this issue. ``` So, we're doing as it suggests for now in case we see any others!
When trying to work with the DB in the tests we were getting ``` Jest did not exit one second after the test run has completed. This usually means that there are asynchronous operations that weren't stopped in your tests. Consider running Jest with `--detectOpenHandles` to troubleshoot this issue. ``` Googling led us to [Having trouble running Jest, unhandled Promises](Vincit/objection.js#1015) and [Need to explicit destroy knex in order to stop the script](Vincit/objection.js#534). The second issue included a comment that setting the pool min size to 0 and idleTimeoutMillis to 500 would solve the problem. We tried it and it worked!
We want to retain the ability to run tests that interact with the DB. But we are also concious that tests are now run asynchronously. So, we can't keep wiping the DB at the start of every test. Instead, we add this global setup file that will be called before Jest runs any tests and it ensure the test DB is clean. We'll then need to take this into account when migrating tests from Lab to Jest.
We wanted to be able to run jest without lots of errors caused by the un-migrated tests. So, specifying what tests to match was the first change we made. From there we solved our DB clean problem by referencing a new global jest setup file. Code coverage is critical for us migrating so that config has been added. What we have at this time ensures we when running tests can see the gaps whilst still providing the lcov file that SonarCloud needs. Finally, we write our tests with the aim that they read as documentation. The best way to see this is when running the tests. The default output removed that ability but adding `verbose: true` means our test output now reads like documentation again.
We relied on a lot of hard coded values in our test helpers. To allow us to run DB tests asynchronously we need to avoid trying to insert multiple records with the same values. This change goes through all our helpers and switches out hard coded ID's for generated ones. Where relevant we also now randomly generate the references used in various records.
We relied on a lot of hard coded values in our test helpers. To allow us to run DB tests asynchronously we need to avoid trying to insert multiple records with the same values. This change goes through all our helpers and switches out hard coded ID's for generated ones. Where relevant we also now randomly generate the references used in various records.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://eaflood.atlassian.net/browse/WATER-4144
For those not aware, the dev team are now doing all new work in water-abstraction-system. This is how we are side-stepping the issues of working with the legacy code base and moving towards a simpler, better future!
When putting together the project we selected Hapi Lab for the unit test framework. The main framework for all Defra Node-based projects is Hapi. So, early Defra projects also selected Lab for unit testing. It was also the framework for Charging Module API so the dev team were familiar with it.
But it has limitations. Other Node unit test frameworks include more functionality that we have to add to Lab ourselves. Documentation as well as online examples are limited. Plus we’ve been keeping an eye on other Defra projects. Nowadays Jest is the framework of choice. All the Future Farming and Rod Licencing repos use it plus anything new we’ve seen appear on GitHub. It probably helps that Jest is the most popular unit test framework for Node. Certainly, most contract devs who join the team are likely to have experience with it. We’d be surprised if they’d even heard of Lab!
So, before we get too far down the path of our bright and shiny future now is the time to look at what it would take to switch to Jest.