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

Move Loaders to this package #3

Closed
wouterj opened this issue Nov 11, 2013 · 8 comments
Closed

Move Loaders to this package #3

wouterj opened this issue Nov 11, 2013 · 8 comments

Comments

@wouterj
Copy link
Contributor

wouterj commented Nov 11, 2013

I did some things to be able to use the config loaders when testing the Configuration: symfony-cmf/Testing#27

@dbu proposed to move those changes to here, what do you think about that?

@matthiasnoback
Copy link
Collaborator

This seems to be a very good addition! Before adding it, I have some thoughts though:

  • Adding file loaders will make this package require symfony/dependency-injection instead of just symfony/config (which currently is not even mentioned in composer.json, I will fix that).
  • It also changes the nature of this package: with it, you can test Configuration classes separately from them being loaded into a service container.
  • Having configuration files associated with the test case class means that you can only use each test class once, while the original implementation of AbstractConfigurationTestCase was meant to test different sets of configuration arrays within the same test class.

I have noticed that many bugs related to my own configuration classes have something to do with the transition from one or more configuration files, to a processed array of configuration values. So I'd be happy to add this functionality. Two ways I think this can be more flexible:

  • symfony/dependency-injection could be a suggested package (or I create a new package SymfonyContainerExtensionConfigTest ;))
  • The logic should be moved to custom constraint classes and the base test class which extends AbstractConfigurationTestCase should have only shortcuts for using the assertions:
abstract class AbstractContainerExtensionConfigurationTestCase extends AbstractConfigurationTestCase
{
    public function assertProcessedConfigurationFromYamlFilesEquals(array $yamlFiles, $expected)
    {
        // ...
    }

}

And maybe even fancier: the loader type could of course be resolved by looking at the extension of a configuration file.

In conclusion: I'd be happy to make the changes required and I can do it tomorrow (of course mentioning you as a contributor to the solution).

@wouterj
Copy link
Contributor Author

wouterj commented Nov 12, 2013

It also changes the nature of this package: with it, you can test Configuration classes separately from them being loaded into a service container.

You can still use them to the test the Configuration classes without the loaders. The 3 default test methods in the CMF testcase should of course be removed.

Having configuration files associated with the test case class means that you can only use each test class once, while the original implementation of AbstractConfigurationTestCase was meant to test different sets of configuration arrays within the same test class.

Sorry, I don't understand this point.

Using a constraint seems pretty cool!

In conclusion: I'd be happy to make the changes required and I can do it tomorrow (of course mentioning you as a contributor to the solution).

That would be great! Otherwise, I can work on it at the end of this week.

@cordoval
Copy link

nice, you guyz rock, anyone documenting this? 👶

@matthiasnoback
Copy link
Collaborator

@wouterj I have been working on this today, the thing is: this feature actually belonged in SymfonyDependencInjectionTest. This package (matthiasnoback/symfony-dependency-injection-test) now has a test class specifically designed for testing different types of configuration files. See https://github.com/matthiasnoback/SymfonyDependencyInjectionTest#test-different-configuration-file-formats for implementation details! The SymfonyConfigTest project will have no extra dependencies now, and thus can be used by anybody who uses just the Config component.

@matthiasnoback
Copy link
Collaborator

By the way, please let me know if this works for you!

@wouterj
Copy link
Contributor Author

wouterj commented Nov 13, 2013

thanks! I'll try it out next week.

@matthiasnoback
Copy link
Collaborator

Thank you too for the excellent suggestion, I think this will turn out as a very useful feature.

@matthiasnoback
Copy link
Collaborator

@cordoval I've added a few lines about this to the README file, if you think it needs anything: please let me know (or create a pull request).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants