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

[3.x] Splitting into a separate TestCase lib? #496

Closed
Jean85 opened this issue Mar 6, 2019 · 46 comments
Closed

[3.x] Splitting into a separate TestCase lib? #496

Jean85 opened this issue Mar 6, 2019 · 46 comments
Milestone

Comments

@Jean85
Copy link
Contributor

Jean85 commented Mar 6, 2019

I started a Twitter thread with @lsmith77 today, because I wanted to fork this repo to extract the only piece of this bundle that I use: the WebTestCase: https://twitter.com/AlessandroLai/status/1103262252411555840

The resulting fork is here: https://github.com/facile-it/symfony-functional-testcase

Lukas later suggested to do the fork "in-house", under the Liip org, so it would be a refactoring of this lib with a simpler extraction: https://twitter.com/lsmith/status/1103374340152872962

He suggested that I open this issue and discuss it here, in particular with @alexislefebvre.

I'm up to it, but only if the resulting lib will have nearly the same features of my fork, or otherwise it will be too much of a hassle for me in respect to my current goal (which is to swap out this bundle for a slimmed down version in my projects).

WDYT?

@alexislefebvre
Copy link
Collaborator

alexislefebvre commented Mar 6, 2019

If I understand correctly, you would like to call some methods of WebTestCase without needing to declare the bundle in AppKernel?

We can extract these methods in a Trait that you would be able to use in your app, without the need to use the whole bundle. Would it fit your needs?

(sorry for closing this issue, the bus made me misclick)

@Jean85
Copy link
Contributor Author

Jean85 commented Mar 6, 2019

That's the biggest part, but obviously having a fork for me is helpful for other small reasons, maintainability as the first: I would have a stable release immediately (we're on alpha since long ago) and I would have to maintain a smaller project with quite a smaller composer.json.

Last but not least, some of the methods that I would like to have (i.e. the assertions on status code) are problematic right now, because they rely on services registered by the bundle, which are missing for me.

I basically don't use 90% of this bundle, so I'm also unable to be fully helpful here to push toward a stable release on all those features, so the fork was the natural response from me.

@alexislefebvre
Copy link
Collaborator

OK, I see, thanks for the explanation.

I'm wondering how splitting the Bundle would complicate the work on it.

Maybe we should better extract everything related to fixtures in another Bundle? We may end up with 2 independent bundles instead of one that require another one.

@dbu
Copy link
Member

dbu commented Mar 7, 2019

Maybe we should better extract everything related to fixtures in another Bundle? We may end up with 2 independent bundles instead of one that require another one.

that would be win-win! the part that @Jean85 did is not a bundle, it basically provides one class that you use in your tests. i don't know what else besides fixtures and the test case is in LiipFunctionalTestBundle - but splitting it up that way and leaving out random stuff that might not even be used by most people sounds like a good plan to me.

@lsmith77
Copy link
Contributor

lsmith77 commented Mar 8, 2019

@alexislefebvre so if we split off the fixture loading stuff you think we can release 2.0 in the next few weeks?

@alexislefebvre
Copy link
Collaborator

@lsmith77 The problems that block the release of 2.0 are related to fixtures: #381 (comment)

So yes we could quickly release 2.0 of this bundle without fixtures.

@lsmith77
Copy link
Contributor

lsmith77 commented Mar 8, 2019

ok so lets explore this .. I think this could be quite beneficial ..

@Jean85 what do you think about this?

@Jean85
Copy link
Contributor Author

Jean85 commented Mar 8, 2019

That's the same for me, but splitting is still a bit of work, especially on your end (new forked repo, decisions on how to handle the namespace, handling deps, refactoring).

@lsmith77
Copy link
Contributor

I am willing to take over the work to creating a new Bundle for the fixture stuff. @alexislefebvre can you take care of wrapping up 2.0 of this Bundle? or how can we coordinate the left over work?

@alexislefebvre
Copy link
Collaborator

I think that we can name the other bundle LiipFixturesBundle. What do you think?

Is it better to fork this repository or create a fresh one?

I can work on splitting the bundle on the next days, I think that it's not that complicated. But maybe I'm missing something since several of you said that it requires some significant work?

@Jean85
Copy link
Contributor Author

Jean85 commented Mar 11, 2019

IMHO splitting the fixtures away from here would be a major BC for the end user, because a lot of features will "disappear" from their POV. Maybe it's better to extract the test case in a separate package, and declare it as a sub-dep of this one? In this way the new package will not be named "bundle", since it wouldn't be one, right?

As for the work, it's about namespacing and reconfiguring CI and integrations.

@lsmith77
Copy link
Contributor

I guess we have 2 ways we can slice things:

  1. non-Bundle stuff vs. Bundle stuff
  2. non-Fixture stuff vs. Fixture stuff

From my understanding @alexislefebvre predicts that 2) is the easier one to achieve quickly. The draw back is indeed bigger disruption for anyone needing all functionality, especially if our fixture solution will come later. Then again they can stick to the 1.x version until then. With the lib approach it would be fairly "hidden" and only people that wanted a reduced set of features can then simply switch to using the lib.

Given how long things have been lingering and given that @alexislefebvre has a high confidence that 2) is doable in a short amount of time, I would rather lean towards 2).

@alexislefebvre alexislefebvre added this to the 2.0 milestone Mar 12, 2019
@alexislefebvre
Copy link
Collaborator

alexislefebvre commented Mar 12, 2019

@Jean85 You're right, I forgot the BC break part. 😅 While I don't like them, I think that it's also a common step in lifecycles of software. So as long as the upgrade instructions are not too long, it's still doable in my opinion.

It would be something like this:

If you need fixtures loading :

  1. composer require --dev liip/fixtures-bundle:~1.0@alpha
  2. replaces use Liip\FunctionalTestBundle\Test\WebTestCase; with use Symfony\Bundle\FrameworkBundle\Test\WebTestCase;
  3. add use Liip\FixturesBundle\…Trait; in your class

(see Panther as an example of how a Trait can be used in a test)

I don't have any opinion about bundle vs. lib since I don't see the consequences for the end user as for now.


I'll try to split the bundle in the next few days and see how much work it requires, on the repository side and while using bundles in a real project.

@lsmith77
Copy link
Contributor

OK thank you!

I will make sure that I will give feedback in a timely manner.

@alexislefebvre
Copy link
Collaborator

alexislefebvre commented Mar 17, 2019

I successfully splitted the fixtures code in another bundle and used them in a project: https://github.com/alexislefebvre/AsyncTweetsBundle/pull/69/files

My project doesn't use all the features of the 2 bundles, so some things may be missing.

I was not easy but it looks like we can split the bundle. 🎉


Here are the 2 bundles:

@lsmith77
Copy link
Contributor

ok cool .. so I will create a LiipFixturesBundle or should it be LiipTestFixturesBundle ?

@alexislefebvre
Copy link
Collaborator

I prefer LiipFixturesBundle personally. LiipTestFixturesBundle is OK too. :)

@dbu
Copy link
Member

dbu commented Mar 18, 2019

there is other fixtures than plain test fixtures. see for example https://symfony.com/doc/master/bundles/DoctrineFixturesBundle/index.html to set up a manual testing system with some fake data. is the LiipFixturesBundle only for automated tests or could it also be used for other things? i'd decide the name based on that question.

@alexislefebvre
Copy link
Collaborator

@dbu You're right, the goal of this bundle will be to load fixtures during test. DoctrineFixturesBundle can be used to load fixtures outside of test environment. So LiipTestFixturesBundle is a perfect name. 👍

@lsmith77
Copy link
Contributor

https://github.com/liip/LiipTestFixturesBundle

@alexislefebvre I gave you commit rights

@lsmith77
Copy link
Contributor

I guess our primary focus should then be to get 2.0 of LiipFunctionalTestBundle shipped .. if you can submit a PR with the fixture stuff removed I will review it ASAP

@alexislefebvre
Copy link
Collaborator

alexislefebvre commented Mar 18, 2019

Thanks @lsmith77 I cloned the 2.x branch as the master branch: https://github.com/liip/LiipTestFixturesBundle

I opened a PR so that everyone can check the changes: liip/LiipTestFixturesBundle#1

Could you please enable Travis CI and Packagist for this repository?

@lsmith77
Copy link
Contributor

ok PR merged and submitted the package to packagist

@lsmith77
Copy link
Contributor

so last steps will be:

  1. moving all fixture related PRs to LiipTestFixturesBundle
  2. removing fixture code from this Bundle
  3. (test) releases for both Bundles

@Jean85
Copy link
Contributor Author

Jean85 commented Mar 25, 2019

3.x is a nice idea! I would suggest to declare that version as abandoned ASAP BTW, or you'll be forced to backport everything always.

@alexislefebvre
Copy link
Collaborator

Should we add LiipTestFixturesBundle as a dependency of the PR so that it doesn't break the current API?

Or should we start the 3.x branch from #502 and ignore the 2.x branch?

Second option seems easier, it means that we would have to change the target of the PR.

@lsmith77
Copy link
Contributor

No, I think with the 3.x strategy we don't need to force this dependency.
and yeah, lets start the 3.x branch right away to keep life simpler.

@lsmith77
Copy link
Contributor

merged .. thx!

awesome work.

I guess we can close this ticket now?

@alexislefebvre
Copy link
Collaborator

alexislefebvre commented Mar 26, 2019

Thanks for the reviews!

Can we release a 3.0.0-alpha1 version of this bundle and a 0.1.0-alpha1 of the fixtures bundle ? I would like to test these versions in a project, it will help to check that they work, and that upgrade and installation guides are valid.

@lsmith77
Copy link
Contributor

sounds good to me

@alexislefebvre
Copy link
Collaborator

alexislefebvre commented Mar 31, 2019

Upgrading was straightforward: https://github.com/alexislefebvre/AsyncTweetsBundle/pull/70/files

I expected that there may be a conflict because getContainer() is defined in both bundles, but it worked.

I opened 2 PRs about documentation:

@alexislefebvre
Copy link
Collaborator

alexislefebvre commented May 30, 2019

Can someone please review some PRs?

I'm working on liip/LiipTestFixturesBundle#4 and if there's no solution, I'll add the workarounds in the documentation.

Then we'll be able to release new stable versions (3.0 for this bundle and 1.0 for LiipTestFixturesBundle).

@alexislefebvre alexislefebvre changed the title [2.x] Splitting into a separate TestCase lib? [3.x] Splitting into a separate TestCase lib? May 31, 2019
@alexislefebvre
Copy link
Collaborator

After releases, we could move issues marked as move-to-fixtures-repository to the TestFixtures repository: https://twitter.com/natfriedman/status/1134167060307750913 (for some reason, I don't see the “Transfer this issue” button).

@lsmith77
Copy link
Contributor

lsmith77 commented May 31, 2019 via email

@lsmith77
Copy link
Contributor

lsmith77 commented Jun 2, 2019

sorry .. looks like I will only get to it tonight or at the latest tomorrow.

@lsmith77
Copy link
Contributor

lsmith77 commented Jun 2, 2019

@alexislefebvre amazing work!

Added a small comment to #506 .. feel free to either adapt the code or not and merge yourself.
liip/LiipTestFixturesBundle#8 needs a rebase and can then be merged

@alexislefebvre
Copy link
Collaborator

Thanks a lot @lsmith77, I'm going to create RC releases in order to test them in my projects, we will be able to publish final releases next week.

@alexislefebvre
Copy link
Collaborator

https://github.com/liip/LiipTestFixturesBundle/releases/tag/1.0.0 🎉

This version is not perfect, there are performance issues, see liip/LiipTestFixturesBundle#12, but at least we now have a stable base to build on.

LiipFunctionalTestBundle is currently broken, we can't release new versions yet, see #520.

@alexislefebvre
Copy link
Collaborator

I created a 1.x branch from current master branch: https://github.com/liip/LiipFunctionalTestBundle/tree/1.x

How can we replace master with 3.x branch? Is it better to open a PR or force push 3.x to master?

@lsmith77
Copy link
Contributor

pushing 3.x into master should work ..
do we have commits in master we do not have in 3.x? if so yeah I guess we need to force push (but maybe first verify that none of the commits in master should be integrated into 3.x)

@alexislefebvre
Copy link
Collaborator

Here it is, the new stable version 3.0.0: https://github.com/liip/LiipFunctionalTestBundle/releases/tag/3.0.0

Thanks everybody who worked on the several versions!

@alexislefebvre
Copy link
Collaborator

@lsmith77 I checked and only one commit from 1.x was missing in last release, I opened a PR about this change: liip/LiipTestFixturesBundle#17

I prefer to avoid force-pushing on master branch so here is a PR to merge 3.x into master: please see #529

@lsmith77
Copy link
Contributor

merged both .. I guess we can then kill 3.x

@alexislefebvre
Copy link
Collaborator

Thanks a lot @lsmith77!

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

No branches or pull requests

4 participants