-
Notifications
You must be signed in to change notification settings - Fork 1
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
[WIP] Cleanup #10
base: master
Are you sure you want to change the base?
[WIP] Cleanup #10
Conversation
That's not quite correct, because page already has access to a Session and PageFactory. Why it needs QATools instance for? If we do this, then Page will depend on QATools as well.
There is no need for that since a single QATools instance is created within extension and then injected into each FeatureContext class that implements needed interface.
And currently there is |
|
||
services: | ||
qa_tools: | ||
class: QATools\BehatExtension\QATools |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a bit funny class name. Maybe we should rename class to something else (last part).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we can keep it as-as because Behat has similar issue with Behat\Behat
class.
Making the factory configurable is good idea. Renaming Regarding the pages and injecting everything I have currently the following problem.
Thats why I we need either a page factory or the
|
Ah that's why the
That should be business of QATools itself according to qa-tools/qa-tools#12
Still not convinced. That would make TypifiedPage version in BehatExtension more powerful then it's counter part in PHPUnit or standalone version. Doesn't sound right. |
Here is the issue about |
Ok, then we need to reintroduce the At all we don't need the |
* Added instance of page factory to Context and removed getPage from QATools
I am not quite sure how to rename |
@@ -8,7 +8,9 @@ | |||
"require": { | |||
"php": ">=5.3.1", | |||
"behat/mink-extension": "~2.0@dev", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, that we should replace dev dependencies to stable analogs, where we have them.
All done. |
Changed to requires to stable version, but added dev-develop to the |
"behat/mink-extension": "~2.0@dev", | ||
"qa-tools/qa-tools": "*@dev" | ||
"behat/mink-extension": "~2.0", | ||
"qa-tools/qa-tools": "dev-develop", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall I change it to ~1.0? or keep it on dev-develop till we released the new version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can set to ~1.0@dev
to allow usage of develop
branch because I bet new PageLocator will be used within this package.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, committed and pushed.
Contains/will contain the following changes:
QATools
instance toPage
instancesQATools
might become singletonQATools
might be injected into a custom ExtensionPage (need to define 3 sub page classes - typified, web and BEM), currently onlyTypifiedPage
supported as default