isVisible fails when element is not present

[richmolj]

According to the documentation, isVisible should return false when the element is hidden OR the element does not exist. But the library currently throws an error when the element does not exist. I believe this is because the code ends up hitting findWithAssert: https://github.com/san650/ember-cli-page-object/blob/master/test-support/page-object/properties/is-visible.js#L16.

[san650]

Hi @richmolj, thanks for taking the time to open the issue.

When we first developed the addon, we thought it was a good idea to throw an
error when the element didn't exist and I still think it's a good idea.

Re-reading the documentation I couldn't find a reference to what you point out:

From https://github.com/san650/ember-cli-page-object/blob/e806c00fbd7e94d96cc0f0b53b16214a0d6c87b5/DOCUMENTATION.md#isvisible

.isVisible
Returns true if the element exists and is visible.

and from the website http://ember-cli-page-object.js.org/api/predicates/#isvisible

.isVisible
Returns true if the element exists and is visible.

It doesn't say that it returns false otherwise, in fact it returns false only if the
element exists but it's not visible, otherwise it throws an error.

Please, let me know if there is another place in the documentation where the
above statement is contradicted so we can fix it.

So, I think this brings up two different issues:

1. The documentation is not clear, we have to work on it! and would be great
   if you can help with that.
2. Is this the expected behavior? Does it follow the principle of
   least surprise? Maybe users would expect it to just return false... I'm not
   sure

Again, thanks for opening the issue and let me know your thoughts

[richmolj]

Thanks for the in-depth reply @san650! To me

Returns true if the element exists and is visible.

It's implied "else it returns false". I can see that's maybe my misunderstanding though.

I'd be happy to improve the docs but IMHO it's not the desired behavior. I like the idea of refactoring a handlebars {{#if}}..{{/if}} to a conditional hidden css class, or vice versa, and not needing to change the tests at all. In addition, if I have the current behavior of isVisible and my test throws an error, this gives me less information...I'm not sure if it's an issue with my selectors or my actual broken logic.

It is fair to say this could lead to false positives...a selector was changed and the logic is broken, but the test still passes since the old selector does not exist. I think this is really the cost of doing business any time you have an {{#if}}...{{/if}} though. Maybe an option you could pass, or an isPresentAndVisible helper?

[lolmaus]

Ran into this problem to.

I believe that there should be .isPresentAndVisible(). I suggest naming it .exists().

Currently, how do I assert that a component does not exist on the page?

[lolmaus]

Okay, this worked (Mocha/Chai):

expect(() => foo().isVisible()).to.throw('Element .foo not found.');

But it's clumsy. I want to do this instead:

expect(foo().exists()).is.false;

[richmolj]

Been using asserting a 'count' of 0 for that purpose myself.

[lolmaus]

The count predicate is only available on collections, not components.

[san650]

@lolmaus:

Currently, how do I assert that a component does not exist on the page?

Right now you can test if a component is hidden on the page using the isHidden property. This property returns true if the property is hidden or it doesn't exist on the page. Is this what you need?

Also, note that every component already includes the isVisible and isHidden predicates, you can override them if you need to set a different selector or scope (see default attributes).

@richmolj:

Maybe an option you could pass, or an isPresentAndVisible helper?

We're discussing changes like this one for the 1.0 release we're working on. The idea is to improve the API by adding, removing and changing methods based on this kind of feedback from the community. Please take a look at this PR which has a list of changes to be done.

I prefer to improve the current properties instead of adding new ones when possible, we could have an option for isVisible to avoid raising an error if the element doesn't exist. Something like

var page = PageObject.create({
  isVisible: isVisible('.a-class', { raise: false }) 
});

Additionally we could have a global configuration to make that option the default behavior, avoiding the need of using that flag every time you use the property.

What do you think?

Personally, I prefer to have the properties to raise an exception when you try to access them, I think it's easier to know what happened, although I agree that the current error message is a bit cryptic. Speaking of which, we have an idea of improving a lot the error messages, one of the reasons we had to start rewriting the internals for the 1.0. Please take a look at Better error messages.

[richmolj]

I agree with improving properties instead of adding new ones 👍.

That said, I'd still think this is incorrect behavior. I expect isVisible to be the opposite of isHidden, which according to your last comment will not raise when the element is not present on the page. If there is an options-based approach, it should apply to both isHidden and isVisible in the same way, and they should function as opposites IMO. If that's not the case, then I would say this is a naming problem that additional configuration options won't fix.

To be clear, I also prefer properties raise when you try to access them and they are not present. And I actually don't have much of an issue with the error messages themselves - makes total sense when you're trying to tell it to click a button that isn't there. My issue is specific to only isVisible since, well, the element isn't visible and it's the logical opposite of isHidden.

[san650]

@richmolj you're right, let's change the behavior so isVisible and isHidden are symmetric. This is going to be implemented for the 1.0 release we're working on.

We can add a raise option later if we find it useful without breaking the compatibility.

Thanks everyone for the insights and feedback, it's really useful to improve the API.

[san650]

Fixed in master