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

Remove dependency on Pimple #21

Closed
adambro opened this issue Nov 6, 2014 · 22 comments
Closed

Remove dependency on Pimple #21

adambro opened this issue Nov 6, 2014 · 22 comments

Comments

@adambro
Copy link

adambro commented Nov 6, 2014

I've tried to use phpunit-mink with my Silex application, but stumbled upon a roadblock. Silex 1.2 (current stable) uses Pimple ~1.0 while phpunit-mink requires Pimple 2.0 or upper. Composer is not able to resolve the conflicts, as Pimple public interface has changed.

My idea is to remove dependency on Pimple in Composer to avoid conflicts. Some ways I see it can be done:

  • copy Pimple code into phpunit-mink repo (quick fix, but hacky)
  • use git submodule to attach Pimple 2.0 code (not sure if Composer supports that)
  • refactor DIContainer so it does not depend on Pimple functionality

Personally I'm in favor of Dependency Inversion, but libraries depending on Dependency Injection Container are hard to integrate due to such issues. Let me know your thoughts.

@aik099
Copy link
Member

aik099 commented Nov 6, 2014

Here is solution, but PR is on you:

  1. change dependency on Pimple 1.0+
  2. create PimpleContainer.php file, that would:
  3. deal with class naming issues:
    • if Pimple\Container class present then extend it (2.x, 3.x)
    • if Pimple\Pimple class present then extend it (1.x, 2.x)
  4. deal with method naming changes
    • in 1.x the default behavior was not to share closures and shared method existed to override that
    • in 2.x+ the default behavior is to share closures and factory method exists to override that
    • create factory method that will ensure new instance each time
    • ensure that adding callback just like that will share it (specially for Pimple 1.x)
  5. extend DIContainer from that PimpleContainer class

That should ensure that we keep public interface of container the same no matter which Pimple version is used beneath it.

@aik099
Copy link
Member

aik099 commented Nov 6, 2014

Fact that Silex doesn't upgrade to Pimple 2/3 doesn't make me any happier either.

@adambro
Copy link
Author

adambro commented Nov 7, 2014

Original Pimple is around 80 LOC while phpunit-mink is using maybe half of its features. Is it really worth to keep that dependency and increase coupling? In this particular scenario (phpunit-mink as independent library) I'd rater go for higher cohesion, even if it means re-implementing few simple features. How about that idea?

@aik099
Copy link
Member

aik099 commented Nov 7, 2014

OK. Then:

  1. upgrade to Pimple 3.0, while we're at it
  2. copy src/Pimple into library/aik099/Pimple (remove tests folder from there)
  3. copy src/Pimple/Tests into tests/aik099/Pimple
  4. update namespaces in files to include aik099\ in front of them
  5. make sure that Pimple tests runs
  6. remove dependency to Pimple & extend from local Pimple copy rather than Composer provided one

@adambro
Copy link
Author

adambro commented Nov 7, 2014

I've just done what you've suggested, but it came with it's own set of problems. Not to mention autoload pain, but Pimple tests is full of checking if service is of particular class, like:

$this->assertInstanceOf('Pimple\Tests\Fixtures\Service', $serviceOne);

I don't think changing all those assertions to match new namespace is wise idea in terms of maintenance.

@aik099
Copy link
Member

aik099 commented Nov 7, 2014

By copy-pasting Pimple code into the library we all understand that it's one way trip and we won't be porting any new Pimple releases (unless some serious bugs will be fixed). I think you safely do find & replace on these.

You can already send PR with what you have for review if you wish. Also make sure coding standards pass, because I bet Pimple uses 4 spaces indentation, but it should be 1 tab instead.

@adambro
Copy link
Author

adambro commented Nov 7, 2014

Crated PR #22 - it is work in progress (failing tests).

Files were just copied, so they adhere to Pimple coding standards. I see that phpunit-mink has got its own coding standards, but I don't know it nor how to check it. Please advise.

@aik099
Copy link
Member

aik099 commented Nov 7, 2014

Any specific reason why you gave me push access to your fork of my repo?

@aik099
Copy link
Member

aik099 commented Nov 7, 2014

I'm using same coding standard on all my projects: https://github.com/aik099/CodingStandard . Please see it's repo for instructions on how to connect and use it.

@adambro
Copy link
Author

adambro commented Nov 7, 2014

Updated contributing instructions with help how to check coding standards, see #23 PR. I guess it is important for you, so better to have it noted :)

Regarding push access to my fork - I might be away for next few days, so it's in case you want to mess with it a bit.

@aik099
Copy link
Member

aik099 commented Nov 7, 2014

I'm in no hurry. I guess next code versions will be after you get back from the place where you're going for a few days.

@adambro
Copy link
Author

adambro commented Nov 11, 2014

It seems that I've misunderstood the value proposition of phpunit-mink
library. I thought it's mere integration code (saving me from writing the
boilerplate integration between PHPUnit and Mink), but apparently it's
rather quite opinionated Selenium-based test runner. That's not exactly
what I was looking for.

On 7 November 2014 16:26, Alexander Obuhovich notifications@github.com
wrote:

I'm in no hurry. I guess next code versions will be after you get back
from the place where you're going for a few days.


Reply to this email directly or view it on GitHub
https://github.com/aik099/phpunit-mink/issues/21#issuecomment-62159759.

@aik099
Copy link
Member

aik099 commented Nov 11, 2014

Any specific problems, that you're experiencing (except Composer dependency issue), that I can help solve besides usual use cases described in http://phpunit-mink.readthedocs.org/en/latest/ ?

The library is basically very simplified version of Behat so you can use Mink with PHPUnit and have your sessions automatically reset after each test or test case. If you're not into Behat but like to test in isolation then library is for you. Support for writing test once and running it in different browsers is quite a unique feature as well.

I took the idea from https://phpunit.de/manual/current/en/selenium.html and replaced Selenium-only restriction with Mink to allow any driver usage. Currently Selenium-only support is implemented due lack of time and not much practical usage from my side (I'm occupied by other projects in my company), but it's really easy to add support for other drivers by changing https://github.com/aik099/phpunit-mink/blob/master/library/aik099/PHPUnit/Session/SessionFactory.php. All parameters from browser configuration are available there.

@aik099
Copy link
Member

aik099 commented Nov 11, 2014

Another thing you can try together with PHPUnit is QA-Tools library (http://www.qa-tools.io) for PageObject approach to UI testing.

@adambro
Copy link
Author

adambro commented Nov 12, 2014

Thanks for your help. Let me provide some context: we're a team new to test automation that is looking for functional test tool for greenfield project. It is typical backoffice web app (mostly forms). Regarding tools we already use PHPUnit for unit tests and reporting test results on Jenkins CI. We're looking at Mink for flexibility: light tests on dev machine (BrowserKit) and full-featured on Jenkins (using Selenium web driver).

Which tool you'd suggest in our case? This QA-Tools lib looks interesting, but not sure how it compares with phpunit-mink tool.

@aik099
Copy link
Member

aik099 commented Nov 12, 2014

This QA-Tools lib looks interesting, but not sure how it compares with phpunit-mink tool.

These are 2 different products. What PHPUnit-Mink does I've explained in https://github.com/aik099/phpunit-mink/issues/21#issuecomment-62630853, but below is short explanation of QA-Tools:

The QA-Tools library is stand-alone product inspired by Selenium Java client using annotations, but Selenium-only solution (not bound to Behat/PHPUnit, only required Mink session obtained from any source to work), which follows PageObject pattern to allow you abstract HTML under test into logically separated methods/classes. It can be used with Behat (there is experimental integration module) and with PHPUnit (PHPUnit-Mink can be used for that)

Example:

  • there is a login form on a page, that contains of 2 inputs (email and password and submit button)
  • in pure Mink world you do:
$form = $session->getPage()->find('css', 'form');
$form->findField('username')->setValue($username);
$form->findField('password')->setValue($password);
$form->findField('submit')->click();
  • in QA-Tools (with PageObject) world you do
$some_page = new SomePage();
$some_page->login($username, $password);

You can see more examples here http://www.qa-tools.io/examples/02-HtmlElements/

Also thanks to annotation you don't need to write much code either, because all selectors (css, xpath, etc.) are written in class property DocBlock and elements are instantiated on demand automatically when you access them.

We're looking at Mink for flexibility: light tests on dev machine (BrowserKit) and full-featured on Jenkins (using Selenium web driver).

I'm using MinkSelenim2Driver/Zombie combination for testing, haven't tried BrowserKit (except for running it's own tests being one of core Mink contributors). But I'm sure that you can change SessionFactory.php a bit, then you'll instantly have BrowserKit support as well. I think this example is pretty self explanatory and allow you to start quickly: http://phpunit-mink.readthedocs.org/en/latest/getting-started.html#basic-usage

@aik099
Copy link
Member

aik099 commented Nov 12, 2014

Mink is too flexible:

  • it searches for element using id/name/title/label, etc., which sometimes can be bad idea to do
  • each found element have all methods, that even don't apply to it (e.g. you can set text of div or submit a div)

QA-Tools is different because it has typified element classes, that only have relevant methods and IDE auto-complete is happy about that.

@aik099
Copy link
Member

aik099 commented Nov 14, 2014

@adambro , is there anything else I can help you with? Have you tried the suggested approach?

@adambro
Copy link
Author

adambro commented Nov 14, 2014

Finally we've went with QA Tools and PageObject - thanks for suggestion. I've managed to write those tests as PHPUnit test cases, so reporting and running is sorted out.

Unless you see value in removing Pimple dependency altogether I'm done with phpunit-mink for now.

@aik099
Copy link
Member

aik099 commented Nov 14, 2014

As I said, if you don't need to test in isolation (session is restarted between tests), then 2 simple setUp and tearDown methods to create and delete session would be just fine.

As for removing Pimple dependency I don't want you to spend time on this, because it clearly would take a lot of time.

As for QA-Tools: the project is way larger then PHPUnit-Mink, but don't have complete documentation yet, except one you see on website and http://docs.qa-tools.io/en/latest/annotations.html page. So if you need more explanation on how things work in there, then feel free to contact me at any time. Please use form at http://www.qa-tools.io/support/ page to ask question instead of creating new issues with questions in them 😉

@aik099
Copy link
Member

aik099 commented Dec 13, 2014

Closing because due lack of development resources.

@aik099 aik099 closed this as completed Dec 13, 2014
@aik099 aik099 reopened this Nov 11, 2015
@aik099
Copy link
Member

aik099 commented Nov 11, 2015

@adambro I've decided to reopen your ticket and redo everything according to plan from #21 (comment) except that I'll call vendor "PimpleCopy" instead of "aik099" because I'm not creating it theoretically.

@aik099 aik099 changed the title [idea] Remove dependency on Pimple Remove dependency on Pimple Nov 11, 2015
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

2 participants