Feature request: provide waitForInvisible etc. to make code clear

[jedwards1211]

No one seeing waitForVisible(..., ..., true) for the first time would think that the true means to actually wait until the given element is invisible, and even I get confused reading or writing my tests sometimes ("why am I waiting for this thing to become visible and not invisible? oh right, the true flag means do the opposite...")

It's a perfect example of unclear code (boolean arguments almost always are). I don't think anyone would prefer the reverse argument over waitForInvisible(...) or waitFor('invisible', ...) or waitForVisibility(..., {visible: false}).

We can of course easily write our own waitForInvisible function, but please keep in mind that because such a function doesn't exist in the webdriver API, not everyone will be inclined to write their own, and a lot of code in the wild will be less readable as a result.

Thoughts?

[MattyBalaam]

Got to agree here, the current API is counter intuitive and requires comments for good code maintainability.

Even worse you also need to also re-declare the timeout.

[christian-bromann]

Even worse you also need to also re-declare the timeout.

I don't see why you have to redeclare the timeout. You can just do $('.hiddenElement').waitForVisible(null, true). I personally don't really see a necessity to add commands for that. Especially since you also just can write it as

browser.waitUntil(() => $('elem').isVisible())

I am open for other opinions.

[MattyBalaam]

OK, using null is a little better, but putting myself in the shoes of anyone who is going to look at the code in future - what exactly does it mean?

I am sprinkling in comments like this at the moment.

.waitForVisible(submit, null, true) //boolean indicates 'waitForInvisible' test.

I'm not sure how the example you have given improves readability unless there was an isInvisible method though?

[plessbd]

I personally have created a few helpers especially because of this https://github.com/ubccr/xdmod/tree/xdmod7.1/open_xdmod/modules/xdmod/automated_tests/webdriverHelpers

I did this so interns and the like would easily be able to just know instead of having to pass a counter intuitive true to waitForVisible. If it was waitForVisible(, false) it would make a bit more sense

But i prefer the waitForInvisible / waitUntilNotExist

However, since we are debating this, I think we should choose one or the other of waitFor vs waitUntil these are the same but having both has always bothered me. I never know which one I am supposed to use

In my opinion, waitUntil > waitFor

[christian-bromann]

In my opinion, waitUntil > waitFor

Not sure what you mean by that but these are just different wait methods. With waitUntil you can wait on arbitrary things whereas waitForVisible obviously only checks for visibility.

My example above was wrong it should be:

browser.waitUntil(() => $('elem').isVisible() === false)

to wait until an element is not visible. My issue with adding commands is that it blows up the API without adding any value. Anyone is free to create helper methods to increase readability.

[MattyBalaam]

Thanks @christian-bromann that's a good solution.

[plessbd]

Not sure what you mean by that but these are just different wait methods. With waitUntil you can wait on arbitrary things whereas waitForVisible obviously only checks for visibility.

That makes sense, I was missing the reasoning.

I just meant from a reading perspective waitUntil looks better than waitFor, though from a writing perspective waitFor is shorter :)

[mmcculloch15]

I think it's pretty undeniably true that elem.waitForInvisible() would be more readable over elem.waitForVisible(null, true). It's also true that most people that come across this in code will need to check the API to figure out what's going on. I think the readability point is a valid one for sure. It's also not something that takes a lot of mental energy once you check out the API for one of the waitForXYZ methods, though. Now that I have that understanding, I do like being able to use the same method for both the regular and reverse scenarios.

If you did go down the route of having separate methods for all of the reverse scenarios, you'd probably also need to include isInvisible() and isNotEnabled() soon afterward, which is getting to ~10 new method additions. I like the helper method solution too, if you want things to be a bit more explicit if it's necessary for your situation.

[MattyBalaam]

My personal preference would definitely be towards an API that would also add the option of passing in parameters along the lines of {reverse: True, timeout: 5000}.

[jedwards1211]

@christian-bromann the bottom line here is you have to admit that someone seeing the following code for the first time:

  await browser.waitForExist('#loginForm', 5000, true)

Would be totally surprised to find out that means "wait for not exist".

[jedwards1211]

@MattyBalaam being able to pass {reverse: true} is slightly better, but it's still horrible from an obviousness standpoint.

[mohithg]

I wrote some helpers functions for this purpose, I named it waitForElementToGo, feel free to use it in your automation scripts https://www.npmjs.com/package/wdio-helpers

[abjerstedt]

@christian-bromann If we aren't going to change the api in V5 - I say we close this issue.

[chrishyle]

Having worked with many developers now on writing WebdriverIO tests, this always comes up, in a couple of ways:

1. A developer writing a test needs the reverse functionality, and doesn't find the boolean argument in API docs, so writes their own waitUntil code directly in the test (issue: discoverability)

• Some developers are super helpful and write a helper for it – which is okay, but I've seen repos with 3-4 helpers all doing the same thing, with slightly different names: waitForNotVisible, waitUntilNotVisible, etc. (issue: duplication, fragmentation)

2. A developer writing a test learns about the boolean argument, but also knows that it is confusing, so adds a comment everywhere they use it – if they're good devs (issue: boilerplate)
3. A developer reading a test does not understand what the boolean means and assumes the obvious "they are waiting for this element to {exist, be visible, etc}" (issue: understandability)

The developers I've seen make these mistake range from totally new to seasoned experts, which is a clear sign to me that the API is broken, it is not an education issue. Good API is discoverable and clearly communicates the developer's intent when they wrote the code; I think the issues I've seen strongly suggest that is not the case here. The ancient software design maxim "code is written once and read many times" is very relevant here.

I am extremely sympathetic to the argument of "don't explode the API", but a) the boolean argument IS part of the API – just a very confusing and poorly-discoverable way to do it, and b) the need to "reverse" these operations is so unbelievably common that having top-level methods for the behaviors is incredibly valuable API (and lacking it is very painful for real-world developers).

I also disagree with @mmcculloch15's slippery-slope argument that adding top-level API to reverse the wait… methods implies we would need to add top-level API to reverse the is… methods: reversing the is… methods is basic boolean inversion behavior built into JS (use a !) ; this is not true of the wait… behaviors which require a custom function to reverse. I am 100% in favor of adding waitForInvisible and 0% in favor of adding isNotVisible.

[abjerstedt]

@chrishyle I just dealt with this EXACT question yesterday in a training session with my devs; and I think you are overthinking this.

We offer addCommand as a feature, and its ridiculously easy to implement your own waitForInvisible(), waitForNotVisible(), or whatever you want to name it.

browser.addCommand('waitForInvisible', function (timeout, message) {
this.waitForDisplayed(timeout,true,message);
},true);

[chrishyle]

@abjerstedt thanks for the quick response! It would definitely not be the first time I over-thought something, but even so, I'm not sure that makes me wrong about this. =P

Implicit in your response is: "you're right, this does come up all the time" – so I think we agree that it is very common, and maybe we even agree that the magic boolean is a bad solution. The real answer you have for this problem is not the boolean, but "write your own helper" – meaning that the "best practice" for every WebdriverIO user, across your entire ecosystem, is to write the code themselves… thus de-facto expanding the API, but in a boilerplate and fragmented way. You have punted the responsibility of solving this to your users =(. Further complicating it is @christian-bromann's fairly consistent messaging that we shouldn't use addCommand… so…

I'm not sure that "maximally minimal" API should be the goal for WebdriverIO (and it already isn't applied consistently – helpers DO exist, this is not a building blocks only framework); I deeply understand the dangers of expanding surface area, but I feel like it is being misapplied here over what I think should be a more important value for this framework: an incredible developer experience writing tests. Having "too much" API hurts that obviously, but so does having "too little" and "flat out bad" API. That you had to explain this in a training yesterday should suggest that you've floated too far in the "too little API" direction, and this feels like an easy fix.

[jedwards1211]

Even the word "reverse" in the API docs is not ideal... it should be "opposite" since "what is the opposite of visible" is more idiomatic than "what is the reverse of visible".

[abjerstedt]

I think addCommand() is considered an accepted feature of webdriverio at this point; and possibly even more accepted in v5 than v4 as the whole logic behind it was rebuilt.

@jedwards1211 how would you feel about going with the chai terminology and call it negate?

[chrishyle]

Ya, naming is maybe weird: waitForNotExist, waitForNotDisplayed, etc. would be more consistent, but less well-formed English. I think I'd slightly favor consistency over grammar. 🤷‍♂️

A functional approach might also be an option:

negate($(selector).waitForExist)();
// or opposite / reverse ?

I think this would still be slightly better than the magic boolean because the intent is more clear when reading it, but I think it would be an unfamiliar pattern in WebdriverIO and still way worse than just $(selector).waitForNotExist().

[jedwards1211]

@abjerstedt I'm used to Chai looking like expect(x).not.to.be(2), do they use negate anywhere in their API or just in the docs?

I'd prefer either @chrishyle's suggestion or getting rid of everything but waitUntil and letting the user use ! in their waitUntil condition.

Or maybe a chai-like API? waitFor('foo > bar').not.to.exist?

[SubJunk]

waitForNotExist is the most intuitive. I think a lot of us have probably tried to use that or looked for it, and then been surprised when it didn't work.

[christian-bromann]

What would you guys think of having a syntax being like:

browser.not.waitForExist()

?

[mgrybyk]

.not stands for inverting waitFor* behavior? Will we wait for still work in old way?

As a temporary solution users who need this function can just definite custom command

[christian-bromann]

.not stands for inverting waitFor* behavior?

Yes. As oppose to e.g. waitForNotExist or waitForNotDisplayed

Will we wait for still work in old way?

What do you mean?

[mgrybyk]

I mean keep ability to pass true/false as a parameter

[christian-bromann]

I mean keep ability to pass true/false as a parameter

Yeah, we would keep this.

[abjerstedt]

Grammar wise it doesn't work. We would need to break it up to be something like
.waitFor.not.displayed().

[christian-bromann]

@jedwards1211 @chrishyle @MattyBalaam comments?

[mgrybyk]

are we still going to add command?

[christian-bromann]

In v6 we changed the structure of these commands to address this issue. Not the command can be called as follows:

$('body').waitForDisplayed() // waits until it is displayed
$('body').waitForDisplayed({ reverse: true }) // waits until it disappears

I hope this is a good compromise everyone can work with.

[jedwards1211]

still not as readable as if there were simply a waitForNotDisplayed method, which could be provided in addition to supporting {reverse: true}

[christian-bromann]

@jedwards1211 you can always add a custom command that does add this command to the element context for better readability.

[jedwards1211]

oh okay