Move location where 'dotenv' is being required #21
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
In a previous commit #10 the 'dotenv' package was moved as early as possible in the application. Unfortunately this has turned out to be a bit too early and is causing some issues when running the unit tests locally. The unit tests don't use the /index.js so 'dotenv' is never required. The 'dotenv' require therefore needs to be moved up a level to /app/server.js.
Cruikshanks
approved these changes
Nov 8, 2022
Cruikshanks
added a commit
that referenced
this pull request
Jan 16, 2023
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.
Cruikshanks
added a commit
that referenced
this pull request
Jan 16, 2023
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 affected 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.
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.
In a previous commit #10 the 'dotenv' package was moved as early as possible in the application.
Unfortunately this has turned out to be a bit too early and is causing some issues when running the unit tests locally. The unit tests don't use the /index.js so 'dotenv' is never required.
The 'dotenv' require therefore needs to be moved up a level to /app/server.js.