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

[WIP] Cleanup #10

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

evangelion1204
Copy link
Contributor

Contains/will contain the following changes:

  • removal of example Move example to a seperate repo #9
  • usage of service container to cleanup DI
  • inject/expose QATools instance to Page instances
    • QATools might become singleton
    • QATools might be injected into a custom ExtensionPage (need to define 3 sub page classes - typified, web and BEM), currently only TypifiedPage supported as default

@aik099
Copy link
Member

aik099 commented Aug 22, 2014

inject/expose QATools instance to Page

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.

** QATools might become singleton ** QATools might be injected into a custom ExtensionPage

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.

(need to define 3 sub page classes - typified, web and BEM), currently only TypifiedPage supported as default

And currently there is TypifedPageFactory in use. Most likely people won't have multiple page factories at same time and we can actually make page factory class a setting in the configuration, which defaults to TypifedPageFactory (put FQCN here).


services:
qa_tools:
class: QATools\BehatExtension\QATools
Copy link
Member

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).

Copy link
Member

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.

@evangelion1204
Copy link
Contributor Author

Making the factory configurable is good idea. Renaming QATools to something more self explaining would give the user a better understanding.

Regarding the pages and injecting everything I have currently the following problem.

  • Pages do have the session but not the page factory, we do not store it
  • doing some action like register with a page it should return the following page as instance like ProfilePage

Thats why I we need either a page factory or the QATools instance.
The QATools instance which would give the following advantage cause the PageFactory

  • has not getPage implemented, which should map Login Pageto namespace\LoginPage or have some custom mapping like login page => namespace\pages\IndexPage
  • allow us to add more help function without adding them to the Page itself, cause they might be specific to the extension

@aik099
Copy link
Member

aik099 commented Aug 22, 2014

doing some action like register with a page it should return the following page as instance like ProfilePage

Ah that's why the Page::getPageFactory() method makes sense. There was one before, but I removed it since it wasn't ever used anywhere except for tests. Bringing it back makes sense, since this way each page can ask page factory that was used to construct it to create another page for it (to support fluid PageObject interface).

has not getPage implemented, which should map Login Pageto namespace\LoginPage or have some custom mapping like login page => namespace\pages\IndexPage

That should be business of QATools itself according to qa-tools/qa-tools#12

allow us to add more help function without adding them to the Page itself, cause they might be specific to the extension

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.

@aik099
Copy link
Member

aik099 commented Aug 22, 2014

Here is the issue about Page::getPageFactory method: qa-tools/qa-tools#84

@evangelion1204
Copy link
Contributor Author

Ok, then we need to reintroduce the getPageFactory method and add support for getPage in the Factory itself, including mapping etc.
Then I can go on with my examples 😄

At all we don't need the QATools instance in the pages. I can't imagine any other things and use cases for now to need the instance in a page besides to create a page.

 * Added instance of page factory to Context and removed getPage from QATools
@evangelion1204
Copy link
Contributor Author

I am not quite sure how to rename QATools currently its just used a lazy instancing of the session and page factory. Later it might contain the support for users and their data as discussed in #5.
Any suggestions?

@@ -8,7 +8,9 @@
"require": {
"php": ">=5.3.1",
"behat/mink-extension": "~2.0@dev",
Copy link
Member

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.

@aik099
Copy link
Member

aik099 commented Feb 8, 2015

#10 (comment)

All done.

@evangelion1204
Copy link
Contributor Author

Changed to requires to stable version, but added dev-develop to the qa-tools/qa-tools dependency till we released 1.2.

"behat/mink-extension": "~2.0@dev",
"qa-tools/qa-tools": "*@dev"
"behat/mink-extension": "~2.0",
"qa-tools/qa-tools": "dev-develop",
Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, committed and pushed.

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.

2 participants