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

Webtestcase: Extract some fixtures methods #271

Conversation

alexislefebvre
Copy link
Collaborator

to the new FixturesLoader class

@alexislefebvre
Copy link
Collaborator Author

alexislefebvre commented Jun 12, 2016

It would be better to move all the code related to fixtures in a new class but it's impossible because it would break the callbacks calls. So this is a compromise.

Here is the corresponding Scrutinizer analysis: https://scrutinizer-ci.com/g/liip/LiipFunctionalTestBundle/inspections/cd1eec10-611e-4f8e-aca7-f02490a88a61 https://scrutinizer-ci.com/g/liip/LiipFunctionalTestBundle/inspections/4fab090d-74e8-49d5-af6a-40725723cde1

@alexislefebvre alexislefebvre force-pushed the webtestcase-extract-fixtures-methods-3 branch from d2fedf3 to ac5f745 Compare June 12, 2016 15:41
@lsmith77
Copy link
Contributor

any specific reason why you made the implementation using static methods? limits the ability to extend if needed.

@alexislefebvre
Copy link
Collaborator Author

No I had no reason to declare the methods as static. But since it calls functions directly like FixturesLoader::locateResources(…);, can FixturesLoader be extended?

@lsmith77
Copy link
Contributor

well if you add a factory method to the base class or make it an optional parameter then yes ..

@alexislefebvre
Copy link
Collaborator Author

I remember now why I declared static methods: it was easier to use them since it doesn't require to instantiate an object before calling the methods from FixturesLoader.

well if you add a factory method to the base class or make it an optional parameter then yes ..

I think that I understand your idea now.

Would adding a method like protected function getFixturesLoader() {} in WebTestCase that

  1. return an instance of FixturesLoader or instantiate an object then return it
  2. can be overridden by any child class

is a good solution?

@lsmith77
Copy link
Contributor

yeah .. I think so

(sorry for the late reply)

@alexislefebvre
Copy link
Collaborator Author

This should be done with traits: #332 (comment)

@alexislefebvre alexislefebvre deleted the webtestcase-extract-fixtures-methods-3 branch March 6, 2018 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants