Fix issue with dotenv in tests #89
Merged
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-3833 In [Refactor dotenv require](#10) it was identified that we were requiring [dotnev](https://github.com/motdotla/dotenv) in multiple places and ideally we should do it once, as early as possible. It was spotted in [Move location where 'dotenv' is being required](#21) that some of the tests were effected by this move, and we bumped it from `index.js` to `/app/server.js`. We then created our first DB migrations and realised we also needed to include it in `config/database.config.js` else migrations wouldn't work. What we now realise is that controller tests work because they require `/app/server.js` which requires **dotenv**. Also, any test that connects with the DB pulls in `config/database.config.js` so those are fine. But when working on [Request new bill run in Charging Module API](request-new-bill-run-in-charging-module) and running tests that need `config/services.config.js` to be populated they were breaking. This is because **dotenv** is never getting `required()` so none of our env vars are getting read in. TL;DR; there was a reason we `require('dotenv')` in all our config files in [sroc-charging-module-api ](https://github.com/DEFRA/sroc-charging-module-api); it's to make both the app _and_ tests work as expected! This change reverts [Refactor dotenv require](#10) to get everything working again.
We don't need to include it here because we know it will have been brought into the app by 1 or more of the config files.
The app won't start without reading in this config so we _know_ `require('dontenv')` will have been run somewhere!
StuAA78
approved these changes
Jan 16, 2023
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-3833
In Refactor dotenv require it was identified that we were requiring dotnev in multiple places and ideally we should do it once, as early as possible.
It was spotted in Move location where 'dotenv' is being required that some of the tests were affected by this move, and we bumped it from
index.jsto/app/server.js.We then created our first DB migrations and realised we also needed to include it in
config/database.config.jselse migrations wouldn't work.What we now realise is that controller tests work because they require
/app/server.jswhich requires dotenv. Also, any test that connects with the DB pulls inconfig/database.config.jsso those are fine.But when working on Request new bill run in Charging Module API and running tests that need
config/services.config.jsto be populated they were breaking. This is because dotenv is never gettingrequired()so none of our env vars are getting read in.TL;DR; there was a reason we
require('dotenv')in all our config files in sroc-charging-module-api ; it's to make both the app and tests work as expected!This change reverts Refactor dotenv require to get everything working again.