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

don't load .env.local in rails' test environment #280

Merged
merged 11 commits into from
Jan 27, 2017

Conversation

siegy22
Copy link
Contributor

@siegy22 siegy22 commented Jan 23, 2017

In my opinion, the .env.local should not overwrite .env.test
variables. When using .env.local for tests, the tests are not
transparent and can't be executed for example on a CI server.

As the documentation says, the .env.local is here for local
overwrites. They should not overwrite a test specific variable. Tests
should be run on every machine in every environment and shouldn't
require any specification of environment variables. (My opinion)

If the .env.local is needed in the test environment, it can be loaded in the test/spec helper:

# test/test_helper.rb
...
Dotenv.load(Rails.root + "env.local")

@siegy22
Copy link
Contributor Author

siegy22 commented Jan 23, 2017

I am struggling with the tests..

I created a second test file to test the dotenv-rails in development mode. But it seems like the Railtie is only initialized once.. What do you think about the tests, what would be the best way to test this?

@bkeepers
Copy link
Owner

Hey @siegy22, thanks for the pull request! I agree that this matches expected behavior.

I created a second test file to test the dotenv-rails in development mode. But it seems like the Railtie is only initialized once.. What do you think about the tests, what would be the best way to test this?

Railties are indeed only initialized once. One idea is to extract the paths to their own method (e.g. dotenv_load_paths, and test just that method with different environments.

In my opinion, the .env.local should not overwrite .env.test
variables. When using .env.local for tests, the tests are not
transparent and can't be executed for example on a CI server.

As the documentation says, the `.env.local` is here for **local**
overwrites. They should not overwrite a test specific variable. Tests
should be run on every machine in every environment and shouldn't
require any specification of environment variables. (My opinion)
@siegy22
Copy link
Contributor Author

siegy22 commented Jan 26, 2017

I rebased once again and adjusted all the tests to the new circumstances.

@siegy22
Copy link
Contributor Author

siegy22 commented Jan 27, 2017

🎉 :shipit:

@bkeepers bkeepers merged commit 0f5e6fa into bkeepers:master Jan 27, 2017
@bkeepers
Copy link
Owner

Thanks @siegy22!

@siegy22 siegy22 deleted the no-local-when-test branch January 27, 2017 15:34
@chuckd
Copy link

chuckd commented Feb 12, 2019

This is completely unexpected behaviour for me. I've run into .env.local not loading in test env a number of times and always assumed it was some bug due to local misconfiguration, not an intentional choice in dotenv-rails! I don't understand the rationale in this PR, for example "can't be executed for example on a CI server": why would you have an .env.local on your CI server? Anyway not a big deal now I know about it, just create an extra .env.test.local with a copy of the env var.

@jakedouglas
Copy link

jakedouglas commented Sep 29, 2019

I don't understand this either.

Tests should be run on every machine in every environment and shouldn't require any specification of environment variables.

At least one very common example of this idea not holding water is that most Rails applications require a database, and the URL of that database may vary between different developer machines, and between developer machines and a CI service. Given this PR, if a developer happens to have a database setup that doesn't match the application's defaults (common), they need to specify the database URL in both .env.local and .env.test.local in order to do local development comprehensively development.

You could argue that every application should come with a docker-compose.yml and so on, such that external service URLs are always consistent, but this is clearly outside of the scope of what dotenv should be prescribing.

@lukeredpath
Copy link

lukeredpath commented Feb 19, 2020

I've also just run into this issue and don't understand the rationale behind it. It was unexpected and confusing. The proposed workaround doesn't help if you have an initialiser that calls Dotenv.require_keys as this will be loaded before you have a chance to call Dotenv.load in your test helper and will raise an error, meaning the only workaround is to ensure you have a .env.test file with your required keys.

IMO this change should be reverted and .env.local should always be loaded in any environment.

As mentioned above, why would you ever need to worry about a .env.local file causing issues on CI? Why are you committing .local files to your repository at all? They are local - they should be in your git ignore file.

ghost pushed a commit to steeple-org/dotenv that referenced this pull request Oct 26, 2020
This basically reverts bkeepers#280.

The above-mentioned PR says:

> When using .env.local for tests, the tests are not transparent and
> can't be executed for example on a CI server.
> As the documentation says, the .env.local is here for local
> overwrites. They should not overwrite a test specific variable. Tests
> should be run on every machine in every environment and shouldn't
> require any specification of environment variables.

While that description matches the expected behavior of Dotenv, the
implementation goes too far (1) and even contradicts other bits of Dotenv's
current behavior (2).

About (1), a [quote][jakedouglas-comment] from the Github user named jakedouglas:

>> Tests should be run on every machine in every environment and
>> shouldn't require any specification of environment variables.
>
> At least one very common example of this idea not holding water is
> that most Rails applications require a database, and the URL of that
> database may vary between different developer machines, and between
> developer machines and a CI service. Given this PR, if a developer
> happens to have a database setup that doesn't match the
> application's defaults (common), they need to specify the database
> URL in both .env.local and .env.test.local in order to do local
> development comprehensively.

About (2), anything that .env.local could do to the test environment
variables before it became ignored for that environment can now be done
with .env.test.local, often at the cost of tediously duplicating
variables from the currently-ignored .env.local. This contradicts the
behavior intended by PR bkeepers#280 and consistency would dictate that
.env.test.local be removed altogether. Only then the test variables
would truly be guaranteed to always stay the same as far as Dotenv is
concerned.

Since (1) shows that local development might require local test
variables for legitimate reasons, removing .env.test.local is not a
valid option and it would make sense to instead restore loading
.env.local for the test environment.

[jakedouglas-comment]: bkeepers#280 (comment)
@codyrobbins
Copy link

I just need to pile on and say that this change is nonsensical, frustrating, and unnecessary and the original rationale behind it doesn’t make any sense to me.

@ktimothy
Copy link

ktimothy commented Dec 30, 2021

Excuse me, @bkeepers, why wouldn't we just revert this piece of illogical code, since most of the community considers it counter-intuitive? =)

@siegy22
Copy link
Contributor Author

siegy22 commented Jan 12, 2022

It's been a while. While I agree with most of you guys, I think it mostly depends on how you use .env.local.
We mostly used the .env.local for overrides that were just relevant when running the application in the development or production mode. As we saw the test environment to be an isolated environment which should not be changed from outside. This way things like "works on my machine" were happening less often. Our .env.test was pretty much defining everything to run the tests on every machine (developers and CI) and didn't include things like a database url because we agreed on a default so every developer could start developing and testing without having to configure a lot of things.

I only see/hear the people here that don't agree with this behaviour because obviously it's been changed with this PR.
No idea how many people agree with this behaviour as we don't hear/see them 😄

However, I feel that this can be reverted as it is a less-opinionated implementation to load the .env.local in the tests.
No hard feelings 😊

feliperaul added a commit to feliperaul/dotenv that referenced this pull request Aug 31, 2023
* Do load .env.local in Rails' test environment

This basically reverts bkeepers#280.

The above-mentioned PR says:

> When using .env.local for tests, the tests are not transparent and
> can't be executed for example on a CI server.
> As the documentation says, the .env.local is here for local
> overwrites. They should not overwrite a test specific variable. Tests
> should be run on every machine in every environment and shouldn't
> require any specification of environment variables.

While that description matches the expected behavior of Dotenv, the
implementation goes too far (1) and even contradicts other bits of Dotenv's
current behavior (2).

About (1), a [quote][jakedouglas-comment] from the Github user named jakedouglas:

>> Tests should be run on every machine in every environment and
>> shouldn't require any specification of environment variables.
>
> At least one very common example of this idea not holding water is
> that most Rails applications require a database, and the URL of that
> database may vary between different developer machines, and between
> developer machines and a CI service. Given this PR, if a developer
> happens to have a database setup that doesn't match the
> application's defaults (common), they need to specify the database
> URL in both .env.local and .env.test.local in order to do local
> development comprehensively.

About (2), anything that .env.local could do to the test environment
variables before it became ignored for that environment can now be done
with .env.test.local, often at the cost of tediously duplicating
variables from the currently-ignored .env.local. This contradicts the
behavior intended by PR bkeepers#280 and consistency would dictate that
.env.test.local be removed altogether. Only then the test variables
would truly be guaranteed to always stay the same as far as Dotenv is
concerned.

Since (1) shows that local development might require local test
variables for legitimate reasons, removing .env.test.local is not a
valid option and it would make sense to instead restore loading
.env.local for the test environment.

[jakedouglas-comment]: bkeepers#280 (comment)

* Clean-up unused fixtures

---------

Co-authored-by: Erwan Thomas <[email protected]>
Co-authored-by: Erwan Thomas <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants