Page MenuHomePhabricator

Selenium pageObjects export constructor vs. new instance
Closed, DeclinedPublic

Description

The selenium JS PageObjects sometime return a class constructor, and sometime return a new instance of that class.

example:
page.js returns a constructor
โ†“
edit.page.js extends the Page class and returns a new instance
โ†“
specs then require() that instance.

This is fine if the Page class is only being extended once, but we can't extend the classes any further after it returns an instance.

example of where this can be a problem (from this patch):
page.js returns a constructor
โ†“
minerva.page extends the Page class and returns a new instance
โ†“
minerva.mobile.page tries to extend that instance
โ†“
๐ŸšจERROR: Class extends value #<MinervaMobilePage> is not a constructor or null๐Ÿšจ

It would be more consistent if all the pageobject classes returned a constructor instead of an instance all the time.
This would however, mean that we would have to change the way the pageobjects are require()'ed in the spec files.
Instead of just requiring the instance like UserLoginPage = require( '../pageobjects/userlogin.page' );
we would have to require the class and create a new instance as well.
Either like
UserLoginPage = new ( require( '../pageobjects/userlogin.page' ) )();
or
UserLoginPage = require( '../pageobjects/userlogin.page' );
userLoginPage = new UserLoginPage();

p.s.
(Looks like this could be related to T185094)

Event Timeline

Restricted Application added a subscriber: Aklapper. ยท View Herald TranscriptMar 1 2018, 10:55 AM

@Jdrewniak TLDR: I am open to making requested change, but I am reluctant to do it.

Our node+selenium framework is by design very simple and tries to follow recommendations of upstream projects. Our current page object pattern is implemented exactly as recommended upstream, see webdriverio page object pattern page.

I am reluctant to diverge from the upstream recommendations because that would mean we have to document our implementation, with all disadvantages that it brings. (Somebody will have to know that we have different implementation than upstream, know where to find the documentation...) I would recommend that you discuss with upstream (issues, twitter, gitter). There might be a good reason for their implementation. Or, your suggestion is better and upstream documentation is updated.

Your right, from the webdriver.io docs

We will always export an instance of a page object and never create that instance in the test.
Since we are writing end to end tests we always see the page as a stateless construct.
http://webdriver.io/guide/testrunner/pageobjects.html

and

Apart from all child page objects Page is created using the prototype model or, using ES6 class

So it looks like their suggestion is to always export an instance of the page object, except for main Page object... :/
unfortunately, the docs don't offer guidance on how to proceed with more complicated models, and there
doesn't seem to be much conversation around that, apart from one article suggestion multiple inheritance
and this other one favouring composition.

unfortunately, the docs don't offer guidance on how to proceed with more complicated models, and there
doesn't seem to be much conversation around that, apart from one article suggestion multiple inheritance
and this other one favouring composition.

I would suggest showing upstream (issues, twitter, gitter) your code and ask them what they recommend for a more complicated cases. I have found upstream really responsive when ever I had a question.

For the record, subclassing is probably not necessary in most situations, but I've been getting more familiar with ES6 classes and realized that subclassing can be achieved without much effort, even when the pageObject returns an instance instead of a constructor. All that's necessary is to create the new pageObject class that extends the parent instance .constructor.

So, in the example in the description, if the class MinervaPage returns new MinervaPage(), you could create a new class called MinervaMobilePage like so:

class MinervaMobilePage extends MinervaPage.constructor {
...
}
Jdrewniak closed this task as Declined.Mar 29 2018, 9:05 PM

Since you have resolved the problem, I would even close it as resolved, not declined. ;)