Even more jest #438
Closed
Even more jest #438
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
https://eaflood.atlassian.net/browse/WATER-4077 Our environments are currently running on Node.js Version 14. [v14 went end-of-life in April 2023](https://endoflife.date/nodejs) which means no more maintenance or security fixes will be provided. The current Long Term Support (LTS) version is currently v18 though in a couple of weeks it will switch to v20. So, we're updating our environments to run with Node v20. This covers all changes needed to support the upgrade. While doing this we spotted that https://github.com/actions/setup-node supports setting the version using the .nvmrc file directly. So, we also used this opportunity to simplify our CI workflow.
https://eaflood.atlassian.net/browse/WATER-4151 In https://eaflood.atlassian.net/browse/WATER-4098 we started to calculate the return volumes, specifically what was inside the return’s abstraction period and what was outside. We wondered what happens when you have a return with a ‘return period’ that crosses two billing periods? For example, return `v1:2:3/456` has a return period of '1 Nov 2021 to 31 Oct 2022’. It reports monthly and the abstraction period is ‘1 Feb to 30 June’. The billing period the engine is considering is ‘1 Apr 2022 to 31 Mar 2023’. The engine matches the return because it crosses over the billing period. Currently, it is then calculating all the volume submitted for the return. But this means it includes submissions for Feb and Mar 2021. We believe it shouldn’t include those submissions. So, when calculating the inside and outside abstraction period volumes for a return, the engine needs to be updated to ignore submissions outside the billing period. This PR will achieve this by filtering out any return lines that are outside of the billing period.
https://eaflood.atlassian.net/browse/WATER-4138 https://eaflood.atlassian.net/browse/WATER-4077 When running the [water-abstraction-acceptance-tests](https://github.com/DEFRA/) recently, we encountered an issue where tests fail because 2 'Big Farm Co Ltd 01' companies are returned when searching for an existing company record. One is a test record, the other is from a previous test. Because the one created in a previous test is not flagged as `is_test` it remains even after running the tear-down. So, we need to tweak the tear-down process for the `crm_v2.companies` table.
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.
Ideally config for tools we use as devs should not be committed unless it's an agreed shared tool we all use.
This is a file VSCode uses to enable additional features. Adding it to gitignore so we can investigate using it to improve the Jest experience.
Get it to run Jest instead of Lab so we can run it using our existing VSCode custom tasks.
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.
|
These changes got merged into #430 |
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.
Some ideas about Investigate switching to jest to discuss before applying to that branch.