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

Added a feature test to demonstrate injection #1

Merged
merged 4 commits into from
Aug 14, 2017
Merged

Added a feature test to demonstrate injection #1

merged 4 commits into from
Aug 14, 2017

Conversation

asgrim
Copy link
Member

@asgrim asgrim commented Aug 9, 2017

No description provided.

@asgrim asgrim self-assigned this Aug 9, 2017
@asgrim asgrim added the WIP label Aug 9, 2017
@asgrim asgrim removed the WIP label Aug 9, 2017
@asgrim asgrim assigned Ocramius and unassigned asgrim Aug 9, 2017
@asgrim asgrim requested a review from Ocramius August 9, 2017 21:44
composer.lock Outdated
@@ -4,25 +4,25 @@
"Read more about it at https://getcomposer.org/doc/01-basic-usage.md#composer-lock-the-lock-file",
Copy link
Member

Choose a reason for hiding this comment

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

The composer.lock should be in .gitignore for a library, since it leads to impossible compatibility scenarios in CI

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed.

@@ -2,7 +2,7 @@

Allows injecting services from a PSR-11-compatibile container in a Behat context.

Created with lots of help from @ciaranmcnulty.
Created with lots of help from [@ciaranmcnulty](https://github.com/ciaranmcnulty).
Copy link
Member

Choose a reason for hiding this comment

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

s/help/swearing

/cc @ciaranmcnulty

* @Given /^I have a Zend\\ServiceManager container$/
*/
public function iHaveAZendServiceManagerContainer()
{
Copy link
Member

Choose a reason for hiding this comment

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

An assertion should be added here, verifying that the container can be injected too

* @When /^I instantiate a context$/
*/
public function iInstantiateAContext()
{
Copy link
Member

Choose a reason for hiding this comment

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

This... is weird

Copy link
Contributor

Choose a reason for hiding this comment

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

Yep, but it's pretty tricky to test Behat extensions - you normally end up resorting to something where you're checking from inside behat that the extension's doing its thing.

A more complete strategy would be to run another Behat process end-to-end, but that's also a bit clunky.

This is better than no tests at least

@@ -2,7 +2,7 @@

Copy link
Member

Choose a reason for hiding this comment

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

This documentation still has the "old way" of instantiating a ServiceManager

Copy link
Member Author

Choose a reason for hiding this comment

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

What do you mean "old" way? That code was taken from a fresh (within the last 2 weeks) Expressive skeleton ...

Copy link
Member

Choose a reason for hiding this comment

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

All tge configuration mumbo-jumbo: the servicemanager just accepts ctor config now

Copy link
Member Author

Choose a reason for hiding this comment

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

Created issue #3 to handle.

Feature:

Scenario:
Given I have a Zend\ServiceManager container
Copy link
Member

Choose a reason for hiding this comment

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

Maybe PSR-11 container?

Copy link
Contributor

Choose a reason for hiding this comment

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

I like the explicitness, it's an example of a container.

Specifically this extension is for 'PSR-11 containers that are returned from an included file' which is not all of them (but applies for Zend)

Copy link
Member

Choose a reason for hiding this comment

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

Alright 👍

@@ -5,7 +5,8 @@
"require": {
"php": "^7.1",
"behat/behat": "^3.3",
"psr/container": "^1.0"
"psr/container": "^1.0",
"symfony/dependency-injection": "^3.3"
Copy link
Contributor

Choose a reason for hiding this comment

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

You should be able to fix the tags instead, in the services.yml - I'll look at that as a separate change after this is merged though

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok thanks, I'll create a separate issue for it

@asgrim asgrim requested a review from Ocramius August 14, 2017 12:02
@asgrim asgrim merged commit 4f5284b into master Aug 14, 2017
@asgrim asgrim deleted the add-tests branch August 14, 2017 12:06
@asgrim asgrim modified the milestone: 1.0.0 Aug 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants