Open aik099 opened 2 weeks ago
Firstly, we shouldn't use $_ENV
or $_SERVER
, we should use getenv()
instead.
Secondly, I'm sorry but I don't understand the solution - what do you want to replace? The linked PR doesn't make a lot of sense to me, since the browser will almost always by different.
Additionally, it's not clear to me why the PHPUnit XML should use <server>
or <env>
- that I cannot judge. My feeling is that using <env>
allows overriding by cli env vars whereas <server>
doesn't allow overriding?
Firstly, we shouldn't use
$_ENV
or$_SERVER
, we should usegetenv()
instead.
I also like getenv
better (not sure though how it is different from $_ENV
). However, it will introduce a BC break for other drivers, because we need to replace $_SERVER
usages in the driver-testsuite
repo as well.
Secondly, I'm sorry but I don't understand the solution - what do you want to replace? The linked PR doesn't make a lot of sense to me, since the browser will almost always by different.
Replace <server name="WEB_FIXTURES_HOST" value="http://host.docker.internal:8002"/>
with a host needed by a GitHub Actions.
Additionally, it's not clear to me why the PHPUnit XML should use
<server>
or<env>
- that I cannot judge. ...
Because this is the only way to configure how test suite runs (e.g. what browser to use for testing, how to map paths inside Document Root and such stuff).
... My feeling is that using
<env>
allows overriding by cli env vars whereas<server>
doesn't allow overriding?
According to PHPUnit documentation using <env....
will change $_ENV
(and getenv
result) unless such environment variable is provided externally.
Example 1:
<env name="WEB_FIXTURES_HOST" value="http://host.docker.internal:8002"/>
in the PHPUnit config filevendor/bin/phpunit
getenv
/ $_ENV
would have WEB_FIXTURES_HOST
key as http://host.docker.internal:8002
Example 2:
<env name="WEB_FIXTURES_HOST" value="http://host.docker.internal:8002"/>
in the PHPUnit config fileWEB_FIXTURES_HOST="my-custom-host:80" vendor/bin/phpunit
getenv
/ $_ENV
would have WEB_FIXTURES_HOST
key as my-custom-host:80
If I had used <server
instead of <env
such a trick wouldn't have worked.
And we want example 2 to work, right? Then the phpunit xml should be using env
, not server
?
I also like getenv better (not sure though how it is different from $_ENV).
$_ENV
is always case sensitive (because of normal PHP array access), but getenv
is case-insensitive depending on OS. e.g. $_ENV['PATH']
breaks on Windows, but getenv('PATH')
is more cross-platform.
getenv
is also more convenient, because you don't have to use isset/empty($_ENV)
.
getenv
is theoretically slower (because of function call), but the benefits are worth it.
And we want example 2 to work, right? Then the phpunit xml should be using
env
, notserver
?
Yes, but to make it work all usages of $_SERVER
in the test suite (not only driver code) must be replaced at least with $_ENV
.
As I said above that would be a BC break for all other drivers using the same test suite.
env variables are readable through $_SERVER
in PHP. We are already relying on that in other drivers when needed: https://github.com/minkphp/MinkSelenium2Driver/blob/db44be5055cb1dfcd323c7b375ff7cf97c187de9/.github/workflows/tests.yml#L87
And we do it also in the current driver: https://github.com/minkphp/webdriver-classic-driver/blob/61b12d05f4185a1f9d0676fc66d559c1e435a97a/.github/workflows/ci.yml#L118-L122
Note also that getenv
is not thread safe. This is not an issue for usages in the testsuite though. but it might be an issue in driver code (as we don't control how it is used)
env variables are readable through
$_SERVER
in PHP. We are already relying on that in other drivers when needed: https://github.com/minkphp/MinkSelenium2Driver/blob/db44be5055cb1dfcd323c7b375ff7cf97c187de9/.github/workflows/tests.yml#L87
This works only, when you don't have corresponding ENV var defined in PHPUnit config file: https://github.com/minkphp/webdriver-classic-driver/blob/61b12d05f4185a1f9d0676fc66d559c1e435a97a/phpunit.xml.dist#L23
This way if we define default env var values in PHPUnit config file they can't overridden via the environment given to PHPUnit runner itself.
Current state:
<server ...
inphpunit.xml.dist
file to define default settings for test suite$_SERVER
in test suite to use the above-defined settingsProblem:
Inability to override the above settings during GitHub Actions builds because we can set environment variables (readable through
$_ENV
) and not server variables (readable through$_SERVER
).Solution
Do so
Find & Replace
in thephpunit.xml.dist
on GitHub Actions instead of providing environment variables likeDRIVER_URL
and such, that don't override whatphpunit.xml.dist
defines.