#[RequiresEnvironmentVariable]#6074
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6074 +/- ##
============================================
+ Coverage 94.57% 94.59% +0.01%
- Complexity 6615 6632 +17
============================================
Files 707 709 +2
Lines 20809 20862 +53
============================================
+ Hits 19681 19734 +53
Misses 1128 1128 ☔ View full report in Codecov by Sentry. |
#[RequiresEnvironmentVariable]
| final readonly class RequiresEnvironmentVariable | ||
| { | ||
| private string $environmentVariableName; | ||
| private null|bool|int|string $value; |
There was a problem hiding this comment.
Environment variable can be strictly only string.
What is the meaning of null, bool, int?
Can I test environment variable presence/absence /wo specific value?
There was a problem hiding this comment.
Based on https://github.com/sebastianbergmann/phpunit/pull/6074/files#diff-8f0e8d461aea381bf08d49440fbc5da892d38eb782518cd44bfe1dddce7638f4R119 it seems bool and int should be removed and as never possible - see https://www.php.net/manual/en/function.getenv.php return type.
There was a problem hiding this comment.
Can I test environment variable presence/absence /wo specific value?
you can only test environment variable presence or with a specific value
it seems bool and int should be removed and as never possible
this was more or less my concern here. I'm also wondering if we should only check the env var in $_ENV , or use getenv() or check at least one of them
There was a problem hiding this comment.
any thoughts @sebastianbergmann? I can provide a patch to my PR
There was a problem hiding this comment.
this was more or less my concern here. I'm also wondering if we should only check the env var in
$_ENV, or usegetenv()or check at least one of them
Please test the behaviour with E2E test and some tests in separate process. The separate process must see the same environment variables in both linux and Windows.
There was a problem hiding this comment.
do you think in the case of "no value", I check if the env var exist, or should I check it is ok-ish? empty string and '0' would not pass
There was a problem hiding this comment.
In my opinion:
-
If an environment variable is tested for a presence (/wo value defined in the Attribute), I would expect
'0'to be consideted as set. -
For binary
'1'/'0'detection I would expect user to write#[RequiresEnvironmentVariable('foo', '1')]explicitly. -
''should be considered as unset as in many operating systemsFOO=unsets the variable. Also, in CI parametrization/matrix empty value might be set, but with "no variable" meaning.
There was a problem hiding this comment.
'' should be considered as unset as in many operating systems FOO= unsets the variable
yeah, good point
Implements #6065