san650 / ember-cli-page-object

This ember-cli addon eases the construction of page objects on your acceptance and integration tests
http://ember-cli-page-object.js.org/
MIT License
275 stars 90 forks source link

Feature Request: Enable inheritence of PageObjects via `extend` #181

Open sohara opened 8 years ago

sohara commented 8 years ago

As discussed in https://github.com/san650/ember-cli-page-object/pull/109, it would convenient for developers using this addon if PageObject shared a similar API to an Ember.Object with respect to allowing a given page object to extend from another page object. This could allow users to easily share code between page objects in a manner similar to that within Ember itself.

Enabling the use of mixins would be another great feature, but I'm guessing that would be more complex to implement.

juanazam commented 8 years ago

@sohara Hi! there are ways to share common code between POs, for example:


const modelComponent = PageObject.create({
  scope: '.modal'
});

const page = PageObject.create({
  modal: modalComponent
});

const otherPage = PageObject.create({
  modal: modalComponent
});
}

I know this is not the same as inheritance but it may solve some issues. Anyway we need to think a way of doing proper inheritance and implement it.

san650 commented 8 years ago

Definitely, this is a feature to implement in the page objects.

Meanwhile you can do what @juanazam suggested and also things like:

import { create } from 'ember-cli-page-object';

export const definition = {
  foo: '...'
};

export default create(definition);

And from another page

import { create } from 'ember-cli-page-object';
import { definition } from './page1';

export default create($.extend(definition, {
  baz: '...',
  qux: '...'
}));
aquamme commented 8 years ago

Along the lines of what @juanazam suggested, it would be nice to be able to use that pattern with collections as well. Something like:

const entryComponent = PageObject.create({
  scope: '.entryComponent'
});

const entriesPage = PageObject.create({
  entries: collection({
    scope: '.entries-container',
    pageObject: entryComponent
  })
});
aquamme commented 8 years ago

@juanazam , is that an established pattern? I'm not seeing any documentation that references composing page objects, and I'm having trouble getting it to work.

This works:

const entriesPage = PageObject.create({
  sortSelector: {
    scope: '.sortSelector',
    buttonIsVisible: isVisible('button')
  }
});

But this does not:

const sortSelector = PageObject.create({
  scope: '.sortSelector',
  buttonIsVisible: isVisible('button')
});

const entriesPage = PageObject.create({
  sortSelector
});
san650 commented 8 years ago

@aquamme Composing PageObject instances doesn't do what you'll expect so it should be avoided.

In order to share code among several page objects you need to use plain javascript objects

e.g.

import { create } from 'ember-cli-page-object';

export const definition = {
  foo: '...'
};

// Page Object 1
export default create($.extend(definition, {
  bar: '...',
  baz: '...'
}));

// Page Object 2
export default create($.extend(definition, {
  baz: '...',
  qux: '...'
}));

You can also use the ES6 notation to merge objects

// Page Object 2
export default create({
  ...definition, // spread "definition" object
  baz: '...',
  qux: '...'
}));

A common pattern would be to define a page object component and export both versions, the plain javascript object and the page object.

import { create } from 'ember-cli-page-object';

export const definition = {
  foo: '...'
};

export default create(definition);

So you'll have the chance to import the definition to use in other page objects or import the actual page object. e.g. import component, { definition } from './pages/components/foo';

Extending and composing page objects is a feature we would like to support in the future, I'll try to work on it over the next weeks.

Hope this help. Let me know if you have more questions.

trisrael commented 7 years ago

Although the above is great, wondering as to why it does not support the same API for these subcomponents.

Using an example from above.

  export default const EntriesPage = PageObject.create({
  sortSelector: {
    scope: '.sortSelector',
    buttonIsVisible: isVisible('button')
  }
});

Trying to use isVisible on a subcomponent.

import EntriesPage from 'tests/pages/entries-page';
...
...
it('has sort selector visible', function() {
  expect(EntriesPage.sortSelector.isVisible).to.be.true;
}

I believe the scope is being applied correctly to the subcomponent, so why do things like isVisible not work as well? We have to add our own isVisible using the same scope selector to get this to work.

san650 commented 7 years ago

@trisrael this looks more like a bug.

For this page object definition

export default const EntriesPage = PageObject.create({
  sortSelector: {
    scope: '.sortSelector',
    buttonIsVisible: isVisible('button')
  }
});

I would expect that EntriesPage.sortSelector.isVisible to generate a selector like:

$('.sortSelector button:visible').length === 0

If that's not the case, then it's a bug.

@trisrael Can you confirm this example is not working? Can you open a separate issue if so? I'll try to create a ember-twiddle to test this.


Update I've created an ember-twiddle and it seems to be working Okay https://ember-twiddle.com/ad62a1b77ff5b5997801d0bb947c271f?fileTreeShown=false&numColumns=2&openFiles=tests.acceptance.my-acceptance-test.js%2Ctemplates.application.hbs The scope is being applied to the isVisible property so the selector is something like

page.subcomponent.isVisible ~= $('.my-scope button:visible').length > 0

Can you give more info about the issue? (in a new issue, please)

trisrael commented 7 years ago

@san650 Hmm very interesting. It does not work locally for me I will check our version of the library :/ Sorry, thanks for quick response and effort.

This is the exact case I was describing. https://ember-twiddle.com/5d89c5211beb9689add87273149f78d7?fileTreeShown=false&numColumns=2&openFiles=tests.acceptance.my-acceptance-test.js%2Ctemplates.application.hbs

juanazam commented 7 years ago

@trisrael hey, I took a look at that twiddle:

assert.ok(page.subcomponent.isVisible, 'button is visible');

That statement is checking that the subcomponent is visible, that is to say an element with selector .container .my-scope is visible (and it is).

The isVisible property called in this case is generated by default, every component support a number of properties out of the box: http://ember-cli-page-object.js.org/docs/v1.6.x/components#default-attributes.

If you want to check if the button within the subcomponent is visible, you should do:

import { test } from 'qunit';
import moduleForAcceptance from '../../tests/helpers/module-for-acceptance';
import _ from 'ember-cli-page-object';

moduleForAcceptance('SubComponent');

var page = _.create({
  scope: '.container',
  visit:  _.visitable('/'),
  subcomponent: {
    scope: '.my-scope',
    btnIsVisible: _.isVisible('button')
  }
});

test('test isVisible', function(assert) {
  page.visit();

  andThen(function() {
    assert.ok(page.subcomponent.btnIsVisible, 'button is visible');
  });
});

Hope this helps

trisrael commented 7 years ago

Sorry, I'm not the best at communicating sometimes. It is working in the twiddles as expected. For some reason in my application at work it does not work as it does in the twiddles for subcomponent.isVisible.

Likely that it is an issue with being behind on versions of ember-cli-page-object or some incorrect setup on our part. Thanks for your time!

san650 commented 7 years ago

@trisrael Please, let us know how it goes. If it's a version issue, please post it here so other users that might have the same problem can find an answer here.

Hope you can fix the issue!

mistahenry commented 6 years ago

I'd love to help on this issue and am curious on your direction. Let me show you the number of ways I'm using inheritance/extension.

I'm exporting pages as POJOs and achieving composition with a custom extend function all over the place in an addon of mine

function extend(){
    return $.extend(true, ...arguments);
}

For the sake of this discussion, imagine you write an addon where you wrap all form elements in a common label + element structure to make bootstrap forms easy:

<div class="form-element-container">
  <label class="control-label col-sm-4">Label</label>
  <div class="form-element col-sm-8">
    <input type="text" class="form-control">
  </div>
</div>

So you represent the sort of abstract form element with:

//base-form-element-page-props.js
const base = {
  scope: ".form-element-container",
  element: {
    scope: ".form-element",
    errorMessages: collection({
       itemScope: ".error-message"
    }),
    formControl: {
      isFormControl: hasClass(".form-control")
    }
  },
  label: {
    scope: ".control-label"
  }
};
export default base;

and then some basic input extends that:

//input-page.js
import FormElementProps from './base-form-element-page-props';
const inputProps = {
  element: {
    formControl: {
      scope: "input",
      fillIn: fillable()
    }
  }
}
export InputPageProps = extend(FormElementProps, InputProps);
export default create(InputPageProps);

Great now I can integration test the component

page.element.formControl.fillIn("name");
assert.equal(page.element.formControl.value, "name");

and then in the context of an acceptance test page object

//login-page.js
import {InputPageProps} from './input-page';

export const LoginPageProps = {
  scope: ".container",
  loginForm: {
    scope: "form",
    username: extend(InputPageProps, {
      scope: "[data-test='username-input']"
    }
  }
}
export default create(LoginPageProps);

I can acceptance test:

page.loginForm.username.element.formControl.fillIn("name");
assert.equal(page.loginForm.username.element.formControl.value, "name");
//or page.loginForm.username.fillIn with some top level aliasing of the component

This has allowed me to achieve all my goals. But, the question is, is this how you want inheritance/extension managed or is there something more idiomatic you'd want. I'm open to discuss / happy to implement

mistahenry commented 6 years ago

While not really having looked at this code base, my initial thought on the matter would be to allow page objects to be passed to the create function (both as the obj and as nested properties). Each node could keep track of the pojo that was passed to it (page.params for the sake of discussion) and simply extend the pojo in the jquery sort of way as mentioned above:


const prop = create({
  scope: "prop-scope"
})
//prop.params = {scope: "prop-scope"}
const base = create({
  scope: "first",
  prop: prop,
  prop2: props.extend({
     scope: "other-scope"
  })
});
// base.params  === {scope: "first", prop: {scope: "prop-scope"}, prop2: {scope: "other-scope"}}
const extender = create(base.extend({
  prop2: prop 
})
//extender.params = {scope: "first", prop: {scope: "prop-scope"}, prop2: {scope: "prop-scope"}}
ro0gr commented 5 years ago

@mistahenry considering your form element example.

Maybe it's just me, but deep overrides makes me harder to reason about the final shape of the definition. In order to get a desirable page object, you have to know the whole nested shape of the source definition, the whole nested shape of the patch definition and the merging algorythm.

That's why when dealing with page object definitions I completely replace all nested objects rather than deep merging it. I believes it just gives more control to my hands and clarity for my eyes.

So my general rule for the defs is to keep nested defintions in separate files (especially for those definition which we are going to re-use or extend). This way we can explicitly extend any level of a resulting page object.

Let me show you what I mean using your form element example. For the sake of clarity I would store defs in the variables rather than moving to files.

So the base definition from your comment would be converted to 3 different defs:

const FormControl = {
    isFormControl: hasClass(".form-control")
}

const Element = {
    scope: ".form-element",
    errorMessages: collection({
        itemScope: ".error-message"
    }),
    formControl: FormControl
}

const Base = {
  scope: ".form-element-container",
  element: Element,
  label: {
    scope: ".control-label"
  }
};

Now I can "extend" each of those defs individually.

const FormInput = Object.assign({}, FormControl, {
  scope: 'input',

  fillIn: fillable()
})

const InputElement = Object.assign({}, Element, {
    formControl: FormInput
})

const MySuperInput = Object.assign({}, Base, {
    element: InputElement
})

I don't think this is ideal, but for now I find it quite natural to do it like this.


You have also provided an example of need to customize a scope on per test basis.

    username: extend(InputPageProps, {
      scope: "[data-test='username-input']"
    }

I agree this is a crucial part of page objects workflow. To handle it I just use plain functions with a required scope first argument and the second optional argument for overrides:

    function Input(scope, overrides = {}) {
        return Object.assign({}, Base, {
            scope,
            element: InputElement
        }, overrides);
    }

This way defining multiple inputs is a pleasure:

const LoginDef = {
    login: Input('[data-test='username-input']'),
    password: Input('[data-test='secret-input']'),
}

Hope this makes sense to you! 😆

mistahenry commented 5 years ago

I don't think this is ideal, but for now I find it quite natural to do it like this.

That's my whole point. I've never said it wasn't possible, just that it was cumbersome, verbose, and unintuitive.

With a shallow merge extend approach + composition, look at the difference:

const element = create({
    scope: ".form-element",
    errorMessages: collection({
        itemScope: ".error-message"
    }),
    isFormControl: hasClass(".form-control")
})

const  bootstrapFormElement = create({
    scope: ".form-element-container",
    element: element,
    label: {
        scope: ".control-label"
    }
});

const inputElement = create(bootstrapFormElement.extend({
    element: element.extend({
        scope: 'input',
        fillIn: fillable()
    })
}))

IMO, everything being a page object is far simpler. Nothing stops you from still externalizing your definitions, always exporting a copy of every definition along side each page object, and using Object.assign + helper functions any time you want to compose.

ro0gr commented 5 years ago

ok, with a shallow extend it doesn't look such a no-go to me.

however, I still think patching the shape/types of nested instances is a potentially undesirable thing to do.

sorry for jumping to semi-classes syntax, but I believe it should describe my fear at the best:

class A {
  nested = new B()
}

class B {
  prop = { text: "" }
}

class C {
  anotherFunnyProp = [ "more fun 💃 " ];
}

const a = new A;

// now we have an object instance and we want little bit different internals 
const newObject = merge(a, {
  nested: new C
})

That's how it looks to me when we talk about page object instance extension. If I saw smth like that in a regular codebase, I'd say there is something wrong with classes hierarchy. And should be solved on a classes level rather than by patching their instances.

patrickberkeley commented 4 years ago

FWIW https://github.com/Addepar/ember-classy-page-object allows this.