#148 Added optionalWaitUntilCondition and optionalWaitWhileCondition #191

georgkunze · 2021-06-03T15:54:53Z

#148 Added optionalWaitUntilCondition and optionalWaitWhileCondition function.

oomelianchuk

Would it also make sense to allow users to pass their own waiting time to the method, analog to Selenide $(element).waitUntil(condition, waitingTime)?

oomelianchuk · 2021-06-04T08:31:57Z

src/main/java/com/xceptance/neodymium/util/SelenideAddons.java

+            try
+            {
+                if (element.has(condition))
+                {
+                    result = true;
+                    break;
+                }
+            }
+            catch (ElementNotFound e)
+            {
+            }


You don't need to catch the ElementNotFound exception here because element.has(condition) doesn't assert anything but just checks if the condition matches.

oomelianchuk · 2021-06-04T08:44:20Z

src/main/java/com/xceptance/neodymium/util/SelenideAddons.java

+            try
+            {
+                if (element.has(not(condition)))
+                {
+                    result = true;
+                    break;
+                }
+            }
+            catch (ElementNotFound e)
+            {
+            }


You don't need to catch the ElementNotFound exception here because element.has(condition) doesn't assert anything but just checks if the condition matches.

oomelianchuk · 2021-06-04T08:49:49Z

src/test/java/com/xceptance/neodymium/util/SelenideAddonsTest.java

+        openBlogPage();
+        SelenideElement privaceDialog = $("#privacy-message");
+        boolean isVisible = SelenideAddons.optionalWaitUntilCondition(privaceDialog, visible);
+        assertTrue(isVisible);


Please, add a message to the assertion, so that in case of test failure, its cause could be easily found

oomelianchuk · 2021-06-04T08:49:54Z

src/test/java/com/xceptance/neodymium/util/SelenideAddonsTest.java

+        long startTime = new Date().getTime();
+        boolean isHidden = SelenideAddons.optionalWaitUntilCondition(privaceDialog, hidden);
+        long endTime = new Date().getTime();
+        assertFalse(isHidden);


Please, add a message to the assertion, so that in case of test failure, its cause could be easily found

oomelianchuk · 2021-06-04T08:50:01Z

src/test/java/com/xceptance/neodymium/util/SelenideAddonsTest.java

+        long startTime = new Date().getTime();
+        boolean isHidden = SelenideAddons.optionalWaitWhileCondition(privaceDialog, visible);
+        long endTime = new Date().getTime();
+        assertFalse(isHidden);


Please, add a message to the assertion, so that in case of test failure, its cause could be easily found

oomelianchuk · 2021-06-04T08:58:41Z

src/test/java/com/xceptance/neodymium/util/SelenideAddonsTest.java

+        boolean isHidden = SelenideAddons.optionalWaitWhileCondition(privaceDialog, visible);
+        long endTime = new Date().getTime();
+        assertFalse(isHidden);
+        Assert.assertTrue(endTime - startTime > Neodymium.configuration().optionalElementRetryTimeout());


Please, add a message to the assertion, so that in case of test failure, its cause could be easily found

oomelianchuk · 2021-06-04T09:02:02Z

src/test/java/com/xceptance/neodymium/util/SelenideAddonsTest.java

+        boolean isHidden = SelenideAddons.optionalWaitUntilCondition(privaceDialog, hidden);
+        long endTime = new Date().getTime();
+        assertFalse(isHidden);
+        Assert.assertTrue(endTime - startTime > Neodymium.configuration().optionalElementRetryTimeout());


use static import like you did in assertTrue(isVisible);

oomelianchuk · 2021-06-04T09:02:19Z

src/test/java/com/xceptance/neodymium/util/SelenideAddonsTest.java

+        boolean isHidden = SelenideAddons.optionalWaitWhileCondition(privaceDialog, visible);
+        long endTime = new Date().getTime();
+        assertFalse(isHidden);
+        Assert.assertTrue(endTime - startTime > Neodymium.configuration().optionalElementRetryTimeout());


use static import like you did in assertTrue(isHidden);

oomelianchuk · 2021-06-04T09:10:38Z

src/test/java/com/xceptance/neodymium/util/SelenideAddonsTest.java

+        boolean isHidden = SelenideAddons.optionalWaitUntilCondition(privaceDialog, hidden);
+        long endTime = new Date().getTime();
+        assertFalse(isHidden);
+        Assert.assertTrue(endTime - startTime > Neodymium.configuration().optionalElementRetryTimeout());


If we know, that the SelenideAddons.optionalWaitUntilCondition(privaceDialog, hidden); should return false, then we can expect, that the method should take maximal waiting time -> Neodymium.configuration().optionalElementRetryTimeout() * Neodymium.configuration().optionalElementRetryCount();. This means, we can improve assert with following code:

Suggested change

Assert.assertTrue(endTime - startTime > Neodymium.configuration().optionalElementRetryTimeout());

int waitingTime = Neodymium.configuration().optionalElementRetryTimeout() * Neodymium.configuration().optionalElementRetryCount();

Assert.assertTrue(Range.between(waitingTime, waitingTime + Neodymium.configuration().optionalElementRetryTimeout()).contains(Math.toIntExact(endTime - startTime)));

where Range comes from org.apache.commons.lang3 package.
What do you think?

oomelianchuk · 2021-06-04T09:11:12Z

src/test/java/com/xceptance/neodymium/util/SelenideAddonsTest.java

+        boolean isHidden = SelenideAddons.optionalWaitWhileCondition(privaceDialog, visible);
+        long endTime = new Date().getTime();
+        assertFalse(isHidden);
+        Assert.assertTrue(endTime - startTime > Neodymium.configuration().optionalElementRetryTimeout());


If we know, that the SelenideAddons.optionalWaitWhileCondition(privaceDialog, visible); should return false, then we can expect, that the method should take maximal waiting time -> Neodymium.configuration().optionalElementRetryTimeout() * Neodymium.configuration().optionalElementRetryCount();. This means, we can improve assert with following code:

Suggested change

Assert.assertTrue(endTime - startTime > Neodymium.configuration().optionalElementRetryTimeout());

int waitingTime = Neodymium.configuration().optionalElementRetryTimeout() * Neodymium.configuration().optionalElementRetryCount();

Assert.assertTrue(Range.between(waitingTime, waitingTime + Neodymium.configuration().optionalElementRetryTimeout()).contains(Math.toIntExact(endTime - startTime)));

where Range comes from org.apache.commons.lang3 package.
What do you think?

oomelianchuk

Sorry, I formulated the comment poorly

Would it also make sense to allow users to pass their own waiting time to the method, analog to Selenide $(element).waitUntil(condition, waitingTime)?

What I wanted to say is, that it would be great to have another method, which would allow to pass the waiting time. These methods would help if the user needs different waiting times at different spots. At the same time, the user should be able to configure the default waiting time (similar to $(element).should(condition))You have adjusted the optionalWaitUntilCondition and optionalWaitWhileCondition to accept waiting time as a parameter and now you can create wrapper methods, which would pass the Neodymium.configuration().optionalElementRetryTimeout() as a waiting time. So we would have:

optionalWaitWhileCondition(SelenideElement element, Condition condition, long waitingTime) analog to $(element).waitWhile(condition, waitingTime)
optionalWaitUntilCondition(SelenideElement element, Condition condition, long waitingTime) analog to $(element).waitUntil(condition, waitingTime)
optionalWaitWhileCondition(SelenideElement element, Condition condition) analog to $(element).shouldNot(condition)
optionalWaitUntilCondition(SelenideElement element, Condition condition) analog to $(element).should(condition)

oomelianchuk

BTW, it would be great, if you could separate the test code into smaller sections using newlines. In this case, the test code would be easier to read and therefore to support. The comments are also appreciated :)

oomelianchuk · 2021-06-07T07:30:01Z

src/test/java/com/xceptance/neodymium/util/SelenideAddonsTest.java

+        long startTime = new Date().getTime();
+        boolean isHidden = SelenideAddons.optionalWaitWhileCondition(privaceDialog, visible);
+        long endTime = new Date().getTime();
+        assertFalse("the privacy message dialog was unexpectedly during within the timeframe", isHidden);


Suggested change

assertFalse("the privacy message dialog was unexpectedly during within the timeframe", isHidden);

assertFalse("the privacy message dialog was unexpectedly hidden during the timeframe", isHidden);

Don't you agree with the suggested changes? Why?

Sorry, did not see. Should be in now.

oomelianchuk · 2021-06-07T08:01:20Z

src/test/java/com/xceptance/neodymium/util/SelenideAddonsTest.java

+        long startTime = new Date().getTime();
+        boolean isHidden = SelenideAddons.optionalWaitWhileCondition(privaceDialog, visible, customWaitingTime);
+        long endTime = new Date().getTime();
+        assertFalse("the privacy message dialog was unexpectedly during within the timeframe", isHidden);


Suggested change

assertFalse("the privacy message dialog was unexpectedly during within the timeframe", isHidden);

assertFalse("the privacy message dialog was unexpectedly hidden within the timeframe", isHidden);

Don't you agree with the suggested changes? Why?

Sorry, did not see. Should be in now.

oomelianchuk · 2021-06-07T08:08:35Z

src/main/java/com/xceptance/neodymium/util/SelenideAddons.java

+        boolean result = false;
+        int counter = 0;
+        while (counter < Neodymium.configuration().optionalElementRetryCount())
+        {
+            counter++;
+            if (element.has(not(condition)))
+            {
+                result = true;
+                break;
+            }
+            Selenide.sleep(waitingTime);
+        }
+        return result;


I think, we can reuse the optionalWaitUntilCondition method here because not(condition) is also a condition

Suggested change

boolean result = false;

int counter = 0;

while (counter < Neodymium.configuration().optionalElementRetryCount())

{

counter++;

if (element.has(not(condition)))

{

result = true;

break;

}

Selenide.sleep(waitingTime);

}

return result;

return optionalWaitUntilCondition(element, not(condition), waitingTime);

oomelianchuk

I think the changes are good now. Great job!

occupant23

@georgkunze Please take care of the comments

occupant23 · 2021-06-10T14:32:22Z

src/test/java/com/xceptance/neodymium/util/SelenideAddonsTest.java

@@ -5,6 +5,8 @@
 import static com.codeborne.selenide.Condition.hidden;
 import static com.codeborne.selenide.Condition.visible;
 import static com.codeborne.selenide.Selenide.$;
+import static org.junit.Assert.assertFalse;


Please remove the static imports for assertFalse and assertTrue and call the functions like

Assert.assert...

Until now we use this scheme at most of our test cases and I would like to have the same approach for similar things.

occupant23 · 2021-06-10T14:35:05Z

src/test/java/com/xceptance/neodymium/util/SelenideAddonsTest.java

+
+        // check that runtime of the wait until method was as long as expected
+        int waitingTime = customWaitingTime * Neodymium.configuration().optionalElementRetryCount();
+        Assert.assertTrue("Runtime was shorter than expected", Range.between(waitingTime, waitingTime + customWaitingTime)


Please adjust the error message to something like
Runtime was not within the expected range
since the runtime can also be longer.

Please refactor this into one private validation method in order to avoid code duplication throughout the 4 test cases. Try to perform the same validation. The other test cases also check with Range.contains

occupant23 · 2021-06-10T14:36:04Z

src/test/java/com/xceptance/neodymium/util/SelenideAddonsTest.java

+        openBlogPage();
+
+        // wait until the optional privacy dialog is visible
+        SelenideElement privaceDialog = $("#privacy-message");


privacyDialog

occupant23

@georgkunze Please take care of the comments

occupant23 · 2021-06-11T05:37:22Z

src/test/java/com/xceptance/neodymium/util/SelenideAddonsTest.java

+        openBlogPage();
+
+        // wait until the optional privacy dialog is not visible anymore, which will fail
+        SelenideElement privaceDialog = $("#privacy-message").shouldBe(visible);


privacyDialog

occupant23 · 2021-06-11T05:37:35Z

src/test/java/com/xceptance/neodymium/util/SelenideAddonsTest.java

+        openBlogPage();
+
+        // wait until the optional privacy dialog is visible
+        SelenideElement privaceDialog = $("#privacy-message");


privacyDialog

occupant23 · 2021-06-11T05:37:44Z

src/test/java/com/xceptance/neodymium/util/SelenideAddonsTest.java

+        openBlogPage();
+
+        // wait until the optional privacy dialog is not visible anymore, which will fail
+        SelenideElement privaceDialog = $("#privacy-message").shouldBe(visible);


privacyDialog

oomelianchuk · 2021-06-14T08:50:31Z

I just thought it might be more convenient for the user if we let them enter a full waiting time instead of a polling interval. What do you think?

occupant23 · 2021-06-17T11:02:54Z

I just thought it might be more convenient for the user if we let them enter a full waiting time instead of a polling interval. What do you think?

@oomelianchuk and @georgkunze I haven't spotted this yet. I just tested the formal side of the code until now. But this sounds very intriguing.
@georgkunze ~~Please adjust the logic accordingly. The polling time can be computed out of the total waiting time and the number of retries.~~

occupant23 · 2021-06-18T10:04:50Z

@georg final decision maxWaitingTime and pollingInterval you can implement a function similar to JavaScriptUtils.until

oomelianchuk · 2021-06-29T14:51:10Z

I think the changes are great now. The only thing I personally would like to change is the type of maxWaitingTime and pollingInterval. Selenide mostly requires waiting time in type long, so it would be more convenient to be able to pass the same type to the optional wait methods. WDYT?

georgkunze · 2021-06-29T15:15:59Z

I think the changes work. There is nothing to add from my side.

occupant23 · 2021-07-05T09:07:32Z

I think the changes are great now. The only thing I personally would like to change is the type of maxWaitingTime and pollingInterval. Selenide mostly requires waiting time in type long, so it would be more convenient to be able to pass the same type to the optional wait methods. WDYT?

@oomelianchuk Good point I adjusted this. Additionally, I will set up a separate issue to refacture the other timings.

#148 Added optionalWaitUntilCondition and optionalWaitWhileCondition

4901d96

georgkunze requested a review from oomelianchuk June 3, 2021 15:54

oomelianchuk requested changes Jun 4, 2021

View reviewed changes

gkunze added 2 commits June 4, 2021 14:25

#148 Improved optional wait functions

0289271

#148 adapted tests

295756f

georgkunze requested a review from oomelianchuk June 4, 2021 12:28

#148 Added missing assertion message

71742cd

oomelianchuk requested changes Jun 4, 2021

View reviewed changes

#148 Added Wrapper for optionalElementRetry functions and tests

c9b8d58

georgkunze linked an issue Jun 4, 2021 that may be closed by this pull request

Create a function to stabilize test that are flaky due to page features that are not relevant for the test #148

Closed

georgkunze requested a review from oomelianchuk June 4, 2021 14:28

oomelianchuk requested changes Jun 7, 2021

View reviewed changes

#148 added comments to test and removed duplicate code

95c566d

georgkunze requested a review from oomelianchuk June 7, 2021 09:15

#148 fixed typo

f475af5

oomelianchuk approved these changes Jun 7, 2021

View reviewed changes

occupant23 suggested changes Jun 10, 2021

View reviewed changes

occupant23 suggested changes Jun 11, 2021

View reviewed changes

#148 removed unneeded static imports and fixed typo

d22b038

georgkunze requested a review from occupant23 June 11, 2021 14:37

occupant23 self-assigned this Jun 14, 2021

#148 Refactore time vailidation in tests

16adc6a

gkunze and others added 3 commits June 18, 2021 16:49

#148 refactored waitUntilCondition to have maxWaitingTime

accc4dc

#148 Expanded test fo waitUntil and waitWhile

576adf4

#148: fixed a small bug, changed config, refactured tests

7f9248e

#148: changed type of waiting times to long

e2efd79

occupant23 approved these changes Jul 5, 2021

View reviewed changes

occupant23 merged commit 9dccc40 into develop Jul 5, 2021

occupant23 deleted the #148-Create-a-function-to-stabilize-test-that-are-flaky-due-to-page-features-that-are-not-relevant-for-the-test branch July 5, 2021 09:23

	Assert.assertTrue(endTime - startTime > Neodymium.configuration().optionalElementRetryTimeout());
	int waitingTime = Neodymium.configuration().optionalElementRetryTimeout() * Neodymium.configuration().optionalElementRetryCount();
	Assert.assertTrue(Range.between(waitingTime, waitingTime + Neodymium.configuration().optionalElementRetryTimeout()).contains(Math.toIntExact(endTime - startTime)));

	assertFalse("the privacy message dialog was unexpectedly during within the timeframe", isHidden);
	assertFalse("the privacy message dialog was unexpectedly hidden during the timeframe", isHidden);

#148 Added optionalWaitUntilCondition and optionalWaitWhileCondition #191

#148 Added optionalWaitUntilCondition and optionalWaitWhileCondition #191

Conversation

georgkunze commented Jun 3, 2021

oomelianchuk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oomelianchuk left a comment

Choose a reason for hiding this comment

oomelianchuk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oomelianchuk left a comment

Choose a reason for hiding this comment

occupant23 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

occupant23 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oomelianchuk commented Jun 14, 2021

occupant23 commented Jun 17, 2021 • edited Loading

occupant23 commented Jun 18, 2021

oomelianchuk commented Jun 29, 2021

georgkunze commented Jun 29, 2021

occupant23 commented Jul 5, 2021

occupant23 commented Jun 17, 2021 •

edited

Loading