[DI] Add tests for EnvVarProcessor #26542
Conversation
| $processor = new EnvVarProcessor($container); | ||
|
|
||
| $result = $processor->getEnv('string', 'foo', function () { | ||
| throw new LogicException('Shouldnt be called'); |
There was a problem hiding this comment.
Minor typo: Shouldnt -> Shouldn\'t or Should not
There was a problem hiding 10000 this comment.
Shouldn't we use $this->fail() instead?
There was a problem hiding this comment.
Yep, I couldn't remember what the function was called, I knew it existed
| return '1'; | ||
| }); | ||
|
|
||
| $this->assertTrue($result); |
There was a problem hiding this comment.
Just asking: assertTrue() accepts as true anything that PHP considers true ... or does it a strict true === ... comparison? We're getting a bool value here, so we need to be sure that a boolean is returned.
There was a problem hiding this comment.
| use Symfony\Component\DependencyInjection\EnvVarProcessor; | ||
| use Symfony\Component\DependencyInjection\Exception\LogicException; | ||
|
|
||
| class EnvVarProcessorTest extends TestCase |
There was a problem hiding this comment.
Just asking: should we add a data provider with some edge cases to better test this feature? Empty strings, null values, zeros, etc. Thanks!
There was a problem hiding this comment.
Agree completely, updated with far more examples for each case
| $processor = new EnvVarProcessor($container); | ||
|
|
||
| $result = $processor->getEnv('string', 'foo', function () { | ||
| throw new LogicException('Shouldnt be called'); |
There was a problem hiding this comment.
Shouldn't we use $this->fail() instead?
| $processor->getEnv('const', 'foo', function ($name) { | ||
| $this->assertSame('foo', $name); | ||
|
|
||
| return 'Symfony\Component\DependencyInjection\Tests\EnvVarProcessorTest::TEST_CONST_OTHER'; |
There was a problem hiding this comment.
I'd suggest to use a name here that tells the reader that you've been using an undefined constant by design here, e.g. TEST_CONST_INVALID or TEST_CONST_UNDEFINED.
a907f07 to
90bc19f
Compare
90bc19f to
532c579
Compare
| array(1), | ||
| array(1.1), | ||
| array(true), | ||
| array(false), |
There was a problem hiding this comment.
I left out null as it would conflict with my other PR, #26498
There was a problem hiding this comment.
Or should I better include it here and update in second PR assuming this gets merged first?
|
@mcfedr I wasn't aware of the current behavior of the base64 decoder. I agree that this PR should not change the behavior. We could tackle this with a later PR, maybe. |
|
@derrabus I guess its one of PHPs fun features, where its just trying to be helpful... |
532c579 to
8e735b7
Compare
|
Thank you @mcfedr. |
This PR was squashed before being merged into the 3.4 branch (closes #26542). Discussion ---------- [DI] Add tests for EnvVarProcessor | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | n/a Add tests for the `EnvVarProcessor` as it doesn't have any at the moment. Originally from this PR against master, #26498 Commits ------- 2992bb3 [DI] Add tests for EnvVarProcessor
Add tests for the
EnvVarProcessoras it doesn't have any at the moment.Originally from this PR against master, #26498