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

Reading .env in PHPUnit versus env variables in phpunit.xml.dist #251

Closed
weaverryan opened this issue Dec 27, 2017 · 20 comments
Closed

Reading .env in PHPUnit versus env variables in phpunit.xml.dist #251

weaverryan opened this issue Dec 27, 2017 · 20 comments

Comments

@weaverryan
Copy link
Member

@weaverryan weaverryan commented Dec 27, 2017

Hi guys!

I noticed that the EnvConfiguration adds environment variables to .env.dist and also phpunit.xml.dist. This is because phpunit does not read .env. Instead, you should define all of your environment variables in phpunit.xml.dist (or set real env variables).

I think this is confusing (I just got bit by it). It means that my environment variables now need to be duplicated in the .env files and also phpunit.xml.dist. It also makes it difficult to run commands in the test environment, as using --env=test isn't enough: the incorrect environment variables will be used (those from .env instead of phpunit.xml.dist). I also didn't know to look in phpunit.xml.dist: I was looking in .env and the app wasn't seeing my changes.

Obviously, this was done intentionally. But, are we sure about this behavior? I would prefer consistency with console and index.php: that .env is read (assuming there is no APP_ENV) and then any environment variables in phpunit.xml.dist override those (admittedly, I'm not exactly sure how to accomplish this).

Cheers!

@javiereguiluz
Copy link
Member

@javiereguiluz javiereguiluz commented Dec 27, 2017

I agree. Let's avoid repetition and let's make phpunit reuse .env contents. If you want to override the value of one or some env vars, then you can use the current idea of defining the new values in the phpunit.xml.dist file.

@sstok
Copy link

@sstok sstok commented Dec 27, 2017

FYI. PHPUnit doesn't (by default) overwrite env variables that are already set. You can use the force="true" option to force this (requires PHPUnit v6.3 - sebastianbergmann/phpunit#2723)

@nicolas-grekas
Copy link
Member

@nicolas-grekas nicolas-grekas commented Dec 27, 2017

"dev" and "test" need very different settings to properly work. I don't think they should be shared between the envs... That could even be dangerous (eg. running tests and destroying "dev" db)
phpunit.xml.dist is the native way to set these, not sure we should create yet-another non-standard way...

@Pierstoval
Copy link
Contributor

@Pierstoval Pierstoval commented Dec 27, 2017

I agree. For example, I'm used to change the database config in test to Sqlite instead of mysql, for faster tests, then I must set different env vars in the phpunit.xml.dist file, which is awesome, because the old parameters.yml system needed me to write everything directly in the config_test.yml, to make sure the test env wouldn't use parameters.yml.

With env vars directly in phpunit.xml.dist, the test env will never use the .env file, so it's perfectly isolated 😄

@weaverryan
Copy link
Member Author

@weaverryan weaverryan commented Dec 30, 2017

Hmm, ok. So if we keep the current situation, we have bad DX. I'm cool with solving that however we can :). Specifically:

A) There is not way to run bin/console commands easily in the test environment (e.g. to initialize the database). That's because the environment variables you set in phpunit.xml.dist are not available for bin/console.

B) There is duplication between .env and phpunit.xml.dist. Usually, 90% of my "dev" values for machine-specific config will be the same for dev and test (e.g. an API key to the sandbox of some service we use). Currently, I need to duplicate this config in 2 places.

C) The behavior is surprising: the kernel/container in the tests works different than all other kernels, including running bin/console --env=test.

What would I suggestion? What if we:

  1. Read .env in the tests. This could be done in KernelTestCase or possibly add code to hook into PHPUnit (which could be activated in phpunit.xml.dist). To be consistent with everything else, this would only be read if APP_ENV is not set.

  2. If users want to override any environment variables in the test environment, they can just do what they always do: set true environment variables or put the values in phpunit.xml.dist themselves. This will prevent .env from being read if you want that control.

This would allow me to do what I've always done: use the same environment variables between dev and test environments, and leverage a committed test-specific config file (e.g. config_test.yml in 3.x) to override the database connection to use a hardcoded SQLite path. No need to have a different set of parameters.

@Pierstoval
Copy link
Contributor

@Pierstoval Pierstoval commented Dec 30, 2017

Let's find solutions:

A) Yeah, we do not often run bin/console in tests, because test env is different. To solve this, I simply call this, either in my tests, or in my bootstrap.php file. Create an Application object, inject a Kernel instanciated in test mode, run commands, debug... This has the big advantage to be blocking when dealing with tests. Even though it needs to write code, I think it's much better, because we can debug this in a much easier way if the test env is broken at compile-time (because it's harder to debug external processes when doing something like exec('php bin/console ...4);

B) Yes, there is duplication. And there may also be some duplicates when dealing with your CI (because you set env vars in your CI, not in versioned files), as well as production env. That doesn't seem like a problem to me. You don't need to duplicate it: flex recipes will add the env vars automatically in your test env, and if it's similar to your dev one, then you will probably never update it, unless you change this API key.

C) Why would the kernel and container working differently than all other kernels be an issue? It's obvious: dev, test and prod are different envs, therefore kernels created with these envs shouldn't work the same way...

For your 1. proposal, I'm not sure it's the best option. It would need a wrapper for the .env-reading part, then we would need to create a bootstrap.php file for PHPUnit that would read the env file, and even then I'm not sure about the precedence (when running PHPUnit, which one is interpreted first: env vars in phpunit.xml or in bootstrap? My intuition would say "the first one", so the xml file, but I don't know 😕 ).
This becomes a problem because we can't use DotEnv (it doesn't overwrite existing env vars), so we need something else... Or we can use the .env file only for vars that are not set in the phpunit.xml files.

For your proposal 2, that's a bit better, but still needs a bootstrap file. Even though I think it's a nice idea to ship such file because we almost always need to setup a project before testing (database, fixtures, etc.) (unless we only do unit tests), I'm not sure implementing a new env system that would read in three places is the best for DX. Then, to debug an incorrect env vars you would have to lookup three types: xml file, env file, global vars. Too much IMO. Only using the xml & global env are enough to me, because xml is for dev, global is for CI. Much simpler, WDYT?

@quentinus95
Copy link

@quentinus95 quentinus95 commented Feb 7, 2018

On my side, the way env variables are managed by Symfony is the main issue I have since I started working with Symfony 4 + flex.

First, I don't completely get why DotEnv variables are only loaded if APP_ENV is not defined. In a normal workflow, I assume env vars will only be loaded from the .env file if we are in dev/test mode because in production, .env files should not be used at all. So we could assume these variables can be loaded if DotEnv is available (i.e. installed) and APP_ENV is not set to prod (to avoid misusages).

Second, I was used to run several commands in test env before running tests (to initiate database schema, for instance) in several projects. The current implementation does not allow this ; I need to run the commands in dev mode or change the conditions in the bin/console (this is the solution I chose). Adding application specific environment variables in the phpunit dist file is not convinent as we should duplicate the vars from the .env file (as stated before). I guess it would be more logical to have only phpunit/testing related env vars (such as SYMFONY_DEPRECATIONS_HELPER) and load application specific env vars from DotEnv if available. This could be done in the bin/phpunit script generated from simple-phpunit (edit: I'm not so sure about this...). We would also keep the current default env vars in the phpunit file (even if I'm not completely convinced by the APP_SECRET variable).

@emmanuelballery
Copy link

@emmanuelballery emmanuelballery commented Feb 8, 2018

I'm currently using a bash script to solve this "ENV vars in test" problem:

set -o allexport # export all defined variables

source .env # load dev parameters
source .env.test # override with test parameters (only those needed)

php bin/console cache:clear -e "test" --no-debug
php bin/console doctrine:database:drop -e "test" --force --no-debug
php bin/console doctrine:database:create -e "test" --no-debug
php bin/console doctrine:migration:migrate -e "test" -n --no-debug
php bin/console doctrine:fixtures:load -e "test" -n --no-debug
# ... and more

php vendor/bin/simple-phpunit | tee tests.txt

This way :

  • APP_ENV is defined and Dotenv is not used in "bin/console"
  • you can keep your "dev" configuration and just override it with what needs to change in "test"
  • you don't need to set any env key in phpunit.xml or phpunit.xml.dist
  • you don't expose those env vars outside this script
@mhujer
Copy link
Contributor

@mhujer mhujer commented Feb 10, 2018

I just got bitten by this. Consider the following use-case:

In tests, I'm importing the fixtures by running doctrine:fixtures:load with the kernel available in KernelTestCase, so it works as expected (it uses the database defined in phpunit.xml.dist).

Now I was a debugging an issue with fixtures loading (caused by incorrect Doctrine mapping), so I tried to run the command directly as I did before Flex (note the --env=test):

bin/console doctrine:fixtures:load --purge-with-truncate --env=test

And it truncated tables and imported the fixtures into the other database (defined in .env).

I'm perfectly aware why this happens (I've read this issue few days ago), but it is still quite unexpected and surprising. It feels like a BC break (radical change in behaviour).

@quentinus95
Copy link

@quentinus95 quentinus95 commented Feb 10, 2018

@emmanuelballery

I have adapted my testing script to source the .env file instead of changing the console source code. However using source is sometimes a problem, such as when running tests in a CI pipeline, where you may need to redefine some env variables (for instance, database connection URI) before running your tests.

EDIT

And another strange behavior: it looks like behat is running in test env but still loads the .env file, which is the opposite behavior of the console file... However, if the APP_ENV var is set to test before running behat, the .env file is not loaded anymore (which is the same behavior as the console file).

The behavior I'm expecting is:

  • Never load a .env file in "prod" env.
  • Always load the .env file in any other env, without overriding existing env vars.
  • Override the env vars with script-specific vars (for instance, vars defined in the phpunit.xml file). I'm not sure about this one, but I guess it would be convenient so we can have some general env vars defined for the test environment and tweak those before running a specific script, depending on which kind of tests we want to run (phpunit/behat/...).
@weaverryan
Copy link
Member Author

@weaverryan weaverryan commented Feb 11, 2018

Fix at symfony/recipes#366 - please review it and make sure it satisfies all of your requirements! :)

@mhujer
Copy link
Contributor

@mhujer mhujer commented Feb 11, 2018

@weaverryan Great improvement! 👏

I was thinking about how to solve the use-case I mentioned three comments above. I came up with following options so far:

  1. create .env.test (maybe better env.test.dist to allow local overriding) and load in the tests bootstrap and also when the bin/console is ran in --env=test

  2. load env vars from phpunit.xml.dist (or phpunit.xml) in when the bin/console is ran in --env=test (very hacky one)

@weaverryan
Copy link
Member Author

@weaverryan weaverryan commented Feb 11, 2018

Yea, we’re discussing the environment-specific .env file idea on the linked PR too. You can share your support there if you agree (there’s no code for this in the PR yet, just a comment of mine)

@alex-barylski
Copy link

@alex-barylski alex-barylski commented Mar 7, 2018

Just curious - I am encountering an error trying to run unit tests that work with repository. I am getting an error Access denied for user 'root'@'localhost' I assume because the "tests" environment is not configured using '.env'? How do I work around this temporarily?

EDIT | I think I figured it out I specified the ENV DATABASE_URL inside phpunit.xml.dist and it seems to work?!?

@pculka
Copy link

@pculka pculka commented Mar 16, 2018

just stumbled upon this and it makes me wonder... WHYYY is this nowhere documented?

@conradkleinespel
Copy link

@conradkleinespel conradkleinespel commented Mar 20, 2018

I've now created a tests/autoload.php file with the following content:

<?php

use Symfony\Component\Dotenv\Dotenv;

require __DIR__.'/../vendor/autoload.php';

if (!isset($_SERVER['APP_ENV'])) {
    if (!class_exists(Dotenv::class)) {
        throw new \RuntimeException('APP_ENV environment variable is not defined. You need to define environment variables for configuration or add "symfony/dotenv" as a Composer dependency to load variables from a .env file.');
    }
    (new Dotenv())->load(__DIR__.'/../.env');
}

And in phpunit.xml.dist, I have this, which just sets test environment specific envvars like APP_ENV:

<?xml version="1.0" encoding="UTF-8"?>

<!-- https://phpunit.de/manual/current/en/appendixes.configuration.html -->
<phpunit xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
         xsi:noNamespaceSchemaLocation="http://schema.phpunit.de/6.1/phpunit.xsd"
         backupGlobals="false"
         colors="true"
         bootstrap="tests/autoload.php"
>
    <php>
        <ini name="error_reporting" value="-1" />
        <env name="KERNEL_CLASS" value="App\Kernel" />
        <env name="APP_ENV" value="test" />
        <env name="APP_DEBUG" value="1" />
        <env name="SHELL_VERBOSITY" value="-1" />
        <!-- define your env variables for the test env here -->
    </php>

    <testsuites>
        <testsuite name="Project Test Suite">
            <directory>tests</directory>
        </testsuite>
    </testsuites>

    <filter>
        <whitelist>
            <directory>src</directory>
        </whitelist>
    </filter>
</phpunit>

Maybe this can help someone keep envvars out of git altogether.

@weaverryan
Copy link
Member Author

@weaverryan weaverryan commented Nov 15, 2018

This is fixed in the latest recipes! Documentation (symfony/symfony-docs#10680) and a blog post coming shortly. From experience, things are MUCH nicer to work with now in the test environment :).

@manu-sparheld
Copy link

@manu-sparheld manu-sparheld commented Sep 13, 2019

@weaverryan Can you please link blog post?

@manu-sparheld
Copy link

@manu-sparheld manu-sparheld commented Sep 16, 2019

@javiereguiluz Thanks 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.