Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[DI] Add tests for EnvVarProcessor#26542
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| $processor = new EnvVarProcessor($container); | ||
| $result = $processor->getEnv('string', 'foo', function () { | ||
| throw new LogicException('Shouldnt be called'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Minor typo:Shouldnt ->Shouldn\'t orShould not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Shouldn't we use$this->fail() instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just asking:assertTrue() accepts as true anything that PHP considers true ... or does it a stricttrue === ... 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| use Symfony\Component\DependencyInjection\EnvVarProcessor; | ||
| use Symfony\Component\DependencyInjection\Exception\LogicException; | ||
| class EnvVarProcessorTest extends TestCase |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Agree completely, updated with far more examples for each case
derrabus left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Forbase64 andjson, I'd suggest to add test cases with badly encoded strings.
| $processor = new EnvVarProcessor($container); | ||
| $result = $processor->getEnv('string', 'foo', function () { | ||
| throw new LogicException('Shouldnt be called'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 orTEST_CONST_UNDEFINED.
a907f07 to90bc19fComparemcfedr commentedMar 16, 2018
90bc19f to532c579Compare| array(1), | ||
| array(1.1), | ||
| array(true), | ||
| array(false), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I left outnull as it would conflict with my other PR,#26498
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Or should I better include it here and update in second PR assuming this gets merged first?
derrabus commentedMar 16, 2018
@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. |
mcfedr commentedMar 16, 2018
@derrabus I guess its one of PHPs fun features, where its just trying to be helpful... |
532c579 to8e735b7Comparenicolas-grekas commentedMar 19, 2018
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/aAdd tests for the `EnvVarProcessor` as it doesn't have any at the moment.Originally from this PR against master,#26498Commits-------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