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

[WIP] Added Config Testing Case #27

Closed
wants to merge 3 commits into from
Closed

[WIP] Added Config Testing Case #27

wants to merge 3 commits into from

Conversation

wouterj
Copy link
Member

@wouterj wouterj commented Nov 11, 2013

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? -
Fixed tickets #23, #9, #5
License MIT
Doc PR tbd

{
abstract protected function getFilenames();
abstract protected function getExpectedResult();
abstract protected function getExtension();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these methods are what users implement in their testing code, so i think they really should have phpdoc on expected return values and semantics.

@dbu
Copy link
Member

dbu commented Nov 11, 2013

assertProcessedConfiguration is part of the symfony base case?

looks nice, although i think we should be careful to not use it too much - testing if a yaml array ends up in the expected array is testing the yaml config component rather than the config file. i guess this test is mainly meant to ensure the Configuration.php class is behaving correctly with fixXmlNames and other post-processing things? where we do that, i think its an excellent idea to test that.

@wouterj
Copy link
Member Author

wouterj commented Nov 11, 2013

assertProcessedConfiguration is part of the symfony base case?

No, it's part of @matthiasnoback's AbstractConfigurationTestCase.

looks nice, although i think we should be careful to not use it too much - testing if a yaml array ends up in the expected array is testing the yaml config component rather than the config file

It should be used once in a bundle, without any other test methods (except for some special cases). It does not test the yaml config component, it tests if the Configuration class matches the expected configuration (yaml, xml and php). It can be used to TDD Configuration classes and to ensure the Configuration class works as expected, without using a functional test. Later on, we can also implement @matthiasnoback's SymfonyDependencyInjectionTest package, to ensure the Extension class does work as expected aswell.

i guess this test is mainly meant to ensure the Configuration.php class is behaving correctly with fixXmlNames and other post-processing things?

This test is indeed also proofing the support for XML for a bundle.

@dbu
Copy link
Member

dbu commented Nov 11, 2013

alright. should this PR then not go against the test of @matthiasnoback directly? this looks very generic to me, not cmf specific at all.

i am fully +1 about starting to test the configuration class and ensure xml support, no doubt about that.

@wouterj
Copy link
Member Author

wouterj commented Nov 11, 2013

I created an issue for that: SymfonyTest/SymfonyConfigTest#3

@wouterj
Copy link
Member Author

wouterj commented Nov 11, 2013

btw, to see an example of this, see the RoutingAutoBundle: symfony-cmf/routing-auto-bundle#36

@wouterj
Copy link
Member Author

wouterj commented Nov 12, 2013

Okay, @matthiasnoback is going to put this in his library. That means this PR waits till that's done and after that, it can be merged with some tweaks.

@wouterj
Copy link
Member Author

wouterj commented Nov 18, 2013

closing this. It's now merged into the SymfonyDependencyInjectionTest library of @matthiasnoback. See symfony-cmf/routing-auto-bundle#38 for an example how to implement this.

@wouterj wouterj closed this Nov 18, 2013
@wouterj wouterj deleted the config_testing branch November 18, 2013 20:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants