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

[DI] highlight suspicious tests #31942

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
3 participants
@nicolas-grekas
Copy link
Member

commented Jun 7, 2019

Q A
Branch? 4.2
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets -
License MIT
Doc PR -

Why doesn't this break something?
Related to #27710 I think
/cc @xabbuh @ro0NL if you have any idea why :)

@@ -340,6 +341,7 @@ class ConfigurationWithArrayNodeRequiringOneElement implements ConfigurationInte
{
public function getConfigTreeBuilder()
{
throw new \Exception();

This comment has been minimized.

Copy link
@ro0NL

ro0NL Jun 7, 2019

Contributor

the test ensures we dont require a node to be present, if the user didnt configure anything. However the side effect is the config tree isnt even initialized ... so it doesnt matter much what happens here.

I'd keep this tree for the purpose of clarity, in the sense we don't expect an InvalidConfigException due nodes being required.

@@ -332,6 +332,7 @@ class EnvConfigurationWithoutRootNode implements ConfigurationInterface
{
public function getConfigTreeBuilder()
{
throw new \Exception();

This comment has been minimized.

Copy link
@ro0NL

ro0NL Jun 7, 2019

Contributor

this test from #27472 is actually invalidated due #27710, we should pass some config to ensure the treebuilder is called

This comment has been minimized.

Copy link
@nicolas-grekas

nicolas-grekas Jun 7, 2019

Author Member

up for a PR?

@ro0NL ro0NL referenced this pull request Jun 7, 2019

Merged

[DI] Fix suspicious test #31944

@nicolas-grekas

This comment has been minimized.

Copy link
Member Author

commented Jun 7, 2019

Closing in favor of #31944, thanks @ro0NL

@nicolas-grekas nicolas-grekas deleted the nicolas-grekas:di-bad-test branch Jun 7, 2019

nicolas-grekas added a commit that referenced this pull request Jun 8, 2019

minor #31944 [DI] Fix suspicious test (ro0NL)
This PR was merged into the 4.2 branch.

Discussion
----------

[DI] Fix suspicious test

| Q             | A
| ------------- | ---
| Branch?       | 4.2
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | #31942
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!-- required for new features -->

cc @nicolas-grekas

Commits
-------

25b961a [DI] Fix suspicious test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.