Support both in PO assertions and out of PO assertions via helper methods

[samvloeberghs]

Hi,

You should not be doing expectations in a page object. That's defeating the purpose of a page object and somewhat bad practice. I saw the generation of the simple example on the website here:

var UnnamedPageObject = function() {

    this.get = function() {
        browser.get('');
    };

    this.a0 = element(by.css('a.a'));

    this.clickA0 = function() {
        this.a0.click();
    };

    this.a0ShouldBeVisible = function() {
        expect(this.a0.isDisplayed()).toBeTruthy();
    };

    this.a0ShouldNotBeVisible = function() {
        expect(this.a0.isDisplayed()).toBeFalsy();
    };

    this.a0ShouldHaveClass = function(className) {
        this.a0.getAttribute('class').then(function(classes) {
            expect(classes.split(' ').indexOf(className) !== -1).toBeTruthy();
        });
    };

    this.a0ShouldNotHaveClass = function(className) {
        this.a0.getAttribute('class').then(function(classes) {
            expect(classes.split(' ').indexOf(className) === -1).toBeTruthy();
        });
    };

};
module.exports = new UnnamedPageObject();

The expectations are typically something you should do in your test file. Not in a helper class. This beats the purpose of seperation of concern.

All that aside, very nice project! :)
Please keep on updating/improving!

Grtz!

[szmeti]

Thanks for the feedback @samvloeberghs.

We know that having expectations in a page object is not a best practice. Or at least according to some Protractor style guides. However people tend to misinterpret that rule.

A page object's primary purpose is to separate the concerns as you pointed out. The two concerns are the following:

• test logic
• how the underlying page is accessed

So the actual tests should describe the test scenario and make assertions while messing with Protractor and HTML should be in the page object.

Our generated page objects do not violate this separation in any way. All the assertions and business logic is still contained within the tests. We just provide an abstract (easier to read) API on top of Protractor and Jasmine. And that is one of the primary goals of a page object, abstraction. If I change from Jasmine to Mocha tomorrow, I do not want to re-write all of my tests just because I followed some best practice that has not been thought through carefully. I just want to update the page objects and leave all of my tests unchanged. Also, what if decide to use native Selenium instead of Protractor (highly unlikely though). In that case, I would need to re-write my tests again.

With our page objects a test looks like the following:

 page.get();
 page.setNewTodo('first protractor test');
 page.clickAddButton();
    
 page.todosCountShouldBe(3);
 page.todoShouldHaveText(2, 'first protractor test');

 page.clickTodoDone(2);
 page.todoShouldHaveClass(2, 'done-true');

The expectations are in the test. This is where we say that the number of todos should be 3, not in the page object. This is where we say that the second todo should have the text 'first protractor test', not in the page object. What we have in the page object is the implementation of the assertion not the assertion itself (how we access the page) and that should not be the concern of the test.

Placing the implementation of the assertions in the test itself also has several disadvantages:

• test cases are difficult to read
• changes in the UI/frameworks breaks multiple tests often in several places
• duplication of expectations both inside and across tests - no reuse
• violation of the Tell Don’t Ask principle (the fields of the page objects (i.e. the located elements) are within the class, but still you are treating them as public members)

So we agree, best practices are good to follow as long as they are based on solid principles. In this case, we do not feel that the separation of concerns rule is well understood within the Protractor community.

[samvloeberghs]

I totally get what you are saying and agree on some things but I don't agree in other things. Don't have the time to elaborate in depth.

"changes in the UI/frameworks breaks multiple tests often in several places". They should only break the page object, not the test. If you put your expectations in your page object, your page object AND your test is broken.

"duplication of expectations both inside and across tests - no reuse". Reuse of expectations implies retesting = bad practices. Unless you make it dynamic, not abstract. But than again these dynamics don't belong in a "page object".

"violation of the Tell Don’t Ask principle (the fields of the page objects (i.e. the located elements) are within the class, but still you are treating them as public members". Your page object should return values or results of logic, no HTML / page elements. I'm thinking about lists of classes, values of input fields, number of elements..

Also did you think about naming? A page object should represent a page, not a test scenario or a simple test/expectation, it is as simple as that. If you really want to follow what you are saying you should just introduce an extra abstraction:

1. your page object
2. your expectation wrapper implementing your page object
3. your test scenario running the ( group of ) expectations

Otherwise it's not a "page object" class anymore, but a bloated "page object to test a specific scenario" class.

Grtz

[samvloeberghs]

I do am in favour of writing a helper function like this:

hasNumberOfElementsWithClass(className: string) {
    // return the number of elements with that class
}

and than in the test file:

expect(page.hasNumberOfElementsWithClass('test')).toEqual(5);

Your argument of having to rewrite your test scenario's if changing from Jasmine to Chai or whatever is also valid the other way round. You will have to update your page objects as well. On the other hand, that's a "get over it and do it" scenario :)

[szmeti]

I guess this is just a matter of taste. I would not say one or the other style is better or worse. Both have advantages and disadvantages. Some people prefer assertions in page objects, some not. Even Martin Fowler mentions the two approaches (http://martinfowler.com/bliki/PageObject.html).

"If you put your expectations in your page object, your page object AND your test is broken." - this is not really true. If my page object has a method like todosCountShouldBe(x), only my page object gets broken, there is nothing in the test that will ever need to be changed.

"Reuse of expectations implies retesting = bad practices" - I do not think this is true either. When I referred to reuse I did not mean retesting. There are assertions, which are not one liners. The a0ShouldHaveClass method in your first code snippet is one such example. I do not want to write that down multiple times in my tests, it is purely an implementation detail. You can say that it can be extracted in to a helper method in the test itself. But what if I wanted to reuse the same assertion in multiple tests? Then you could say, extract it into a helper class. But then you would need to pass in to that helper method the element located by the page object, which would again lead to exposing the page object field as a public field.

"Your page object should return values or results of logic, no HTML / page elements." - this sounds good in theory, but it cannot always be done in practice. Protractor helpers like isDisplayed, isPresent, isSelected are examples that require you to expose the elements if the assertions are not in the page objects. Or you can implement the same methods for each element in your PO just to avoid exposing the elements directly (e.g. a0IsDisplayed and then in your test expect(page.a0IsDisplayed()).toBeTrue()), but that seems an awful lot of duplication to me.

"Otherwise it's not a "page object" class anymore, but a bloated "page object to test a specific scenario" class." - When we generate a page object, we do not know anything about your business logic. All we get from the user is an HTML fragment. So in that sense, the generated code that contains assertions cannot possibly test only a specific scenario, because we have not got a clue about test scenarios. It is a generic class that can be used in any context.

"Your argument of having to rewrite your test scenario's if changing from Jasmine to Chai or whatever is also valid the other way round. You will have to update your page objects as well." - except that I have a generator that will regenerate it for me and it will only take 10 seconds :) (not as of today, but we have plans to support other languages/frameworks).

By the way, we do provide checkboxes on our website to disable assertions if you do not like them.

The important take away here is that the tool should support both approaches and let the user decide which one he prefers. Essentially, we should extract the helper methods that you prefer from our assertion methods and let the assertion methods use those helpers instead. I rename the issue title to reflect that.

[mmmichl]

First off, I like the project and what it offers!

To jump right into the discussion: I did not made up my mind, yet, but I lean more to the direction to not have assertion and helper methods. To make the code more readable in the spec files, I am using helper libraries to pimp my selectors and matchers: https://www.npmjs.com/package/protractor-helpers

[szmeti]

We do realise that some developers like to have smaller POs and they just implement the assertions themselves in the test. But we also think it is important to support both styles. I also thought about introducing a helper library. That way there would be less duplication in the POs.

I did not know there is already a similar library. Thanks for pointing it out. I will check it out. We are currently refactoring the code base to make adding support for other styles easier. So stay tuned, there will be changes in this area soon.