Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add method $.shouldBe(condition, timeout) #1136

Closed
asolntsev opened this issue Apr 26, 2020 · 14 comments · Fixed by #1340
Closed

Add method $.shouldBe(condition, timeout) #1136

asolntsev opened this issue Apr 26, 2020 · 14 comments · Fixed by #1340

Comments

@asolntsev
Copy link
Member

The problem

Selenide has method for waiting for a condition with custom timeout:

  • $.waitUntil(visible, 8888);
  • $.waitWhile(visible, 9999);

But it's not always readable. See $.waitUntil(attribute("href", "blah"), 8888);.

Solution

It would be better readable if method name was should/shouldBe/shouldHave instead of waitUntil (but with timeout parameter):

  • $.shouldBe(visible, 8888);
  • $.shouldHave(attribute("href", "blah"), 9999);

P.S. Selenide already uses this pattern for collections:

  • $$("tr").shouldHave(size(42));
  • $$("tr").shouldHave(size(42), 9999);

Tell us about your environment

  • Selenide Version: 5.11.1
@asolntsev asolntsev self-assigned this Apr 26, 2020
@dstekanov
Copy link
Member

I think we should have these methods. Also, shoulds accept Condition... but waitUntil does not

@asolntsev
Copy link
Member Author

@dstekanov There is a nuance :)
In Java, you cannot add long parameter after vararg: $.shouldBe(visible, enabled, 9999);

That's why I suggest to have "waiting" methods with only one condition. As a rule, you want to use big timeout to wait for a single condition.

@SeleniumTestAB
Copy link
Contributor

SeleniumTestAB commented Apr 28, 2020

Hmm why not sometihng like that element.shouldBe(visible, enabled).after(timeout); ?
OR
element.after(timeout).shouldBe(Condition...)

@asolntsev
Copy link
Member Author

@SeleniumTestAB Good idea, but...

  1. element.shouldBe(visible, enabled).after(timeout) - cannot work, because the first part element.shouldBe(visible, enabled) already does its job.
  2. But these options are possible:
  • element.after(timeout).shouldBe(Condition...)
  • element.shouldBe(Condition...)
  • element.shouldBe(visible.in(5, SECONDS), enabled.in(8, SECONDS)

@SeleniumTestAB
Copy link
Contributor

hmm, why not not do element.shoulBe(visible.in(Duration duration)) ? Like i am not sure if you really need to specify the time measure... unless its complex to handle it that way,

@asolntsev
Copy link
Member Author

I agree, the unit is almost always SECONDS, but without the unit it's not clear what `in(5) means:

element.shoulBe(visible.in(5))

@SeleniumTestAB
Copy link
Contributor

yes thus i mentioned Duration class from java so that we can use :
element.shouldBe(visible.in(Duration.ofMiniutes(5));

Thats much readable but problem is to how Selenide can check if its in Minutes or Seconds... hmm i think for starter we could set it for Seconds and maybe later see feedback from community?
@asolntsev

@SeleniumTestAB
Copy link
Contributor

@asolntsev there is one issue thought with Condition.in(timeout) method. The conditions such as exist or visible are a static variables and adding in() to them wouldnt make it change timeout for not one occurance but all occurances in the code?

@asolntsev
Copy link
Member Author

No, it's not a problem.
Look how method Condition.because is implemented: it always returns a new Condition instance.

@SeleniumTestAB
Copy link
Contributor

So the finall thing should be:
element.shouldBe(visible.after(Duration timeout));
in my opinion if we stick to duration, however if we want to explicity say the type of time measure then we can use the aforementioned form and:
element.shoulHave(attribute("someAttribute", "someValue").in(4, SECONDS));

@raidora
Copy link

raidora commented Dec 1, 2020

I hope there isn't a rule against necroing a thread.

My 2 cents go in favour of using Duration objects as well. int, temporalunit parameters almost feel like a hack next to it.
And you save the hassle of internally having to keep track of the provided temporalunit.

The users still retain the ability to input the integer in whatever unit they wish through Java's Duration class.
Duration.ofMinutes(2); Duration.ofSeconds(10); Duration.ofMillis(1450) etc.

Using Duration for timeouts in Selenide might promote proper time unit usage throughout the users' test code as well.
Maybe one day we won't find timeouts like 5000 or 5*1000 in test projects using Selenide.

@asolntsev
Copy link
Member Author

@raidora Sure, any help is appreciated.
Yes, I agree that using TemporalUnits seems to be a good choice.

asolntsev added a commit that referenced this issue Dec 2, 2020
... as a replacement for $.waitUntil(condition, timeout)
@asolntsev
Copy link
Member Author

@raidora Please take a look at #1340

I added methods $.shouldBe(Condition, Duration) and $.shouldHave(Condition, Duration) as a replacement for $.waitUntil and $.waitWhile.

The only thing that I am in doubt about: we don't have alternative for method waitUntil(Condition condition, long timeoutMilliseconds, long pollingIntervalMilliseconds). But probably people never use this third parameter pollingIntervalMilliseconds (I hope so)?

asolntsev added a commit that referenced this issue Dec 3, 2020
... as a replacement for $.waitUntil(condition, timeout)
asolntsev added a commit that referenced this issue Dec 7, 2020
asolntsev added a commit that referenced this issue Dec 8, 2020
... as a replacement for $.waitUntil(condition, timeout)
asolntsev added a commit that referenced this issue Dec 8, 2020
@asolntsev asolntsev linked a pull request Dec 8, 2020 that will close this issue
@asolntsev asolntsev added this to the 5.17.0 milestone Dec 8, 2020
@asolntsev
Copy link
Member Author

implemented in #1340

asolntsev added a commit that referenced this issue Jan 20, 2021
akoira pushed a commit to akoira/selenide that referenced this issue Jun 13, 2021
... as a replacement for $.waitUntil(condition, timeout)
akoira pushed a commit to akoira/selenide that referenced this issue Jun 13, 2021
akoira pushed a commit to akoira/selenide that referenced this issue Jun 13, 2021
akoira pushed a commit to akoira/selenide that referenced this issue Jun 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants