ctubbsii commented on code in PR #3508:
URL: https://github.com/apache/accumulo/pull/3508#discussion_r1240320035
##########
test/src/main/java/org/apache/accumulo/test/util/Wait.java:
##########
@@ -48,4 +50,13 @@ public static boolean waitFor(final Condition condition,
final long duration,
}
return conditionSatisfied;
}
+
+ /**
+ * A retry for use in junit tests when testing asynchronous conditions that
are expected to
+ * eventually meet the condition or fail the test. Using this method should
be used instead of an
+ * arbitrary sleep and hoping to catch the condition in the expected state.
+ */
+ public static void assertTrueWithRetry(final Wait.Condition condition)
throws Exception {
+ assertTrue(waitFor(condition));
+ }
Review Comment:
I guess I have few thoughts here that superseded my previous comment:
1. My suggestion was mainly about reducing boilerplate. Without the custom
timeouts, there's already a lot less boilerplate and the method might not be
needed.
2. Your name is longer than the one I suggested, making it less beneficial
in my mind.
3. My original suggestion was to create a local method in the test, not in
the Wait test utility class, because there seemed to be a convention to avoid
using the assertions in the Wait class, for some reason. I'm not opposed to
putting the assertions directly in the Wait class, but it wasn't done before.
If you're willing to put the assertions in the Wait class, then it could be
done with the existing methods.
4. After looking more closely at the Wait class, and how it is used, I think
all uses of it would benefit from having the assertions checked inside its
waitFor methods. If that's done (can be a separate PR), then adding this new
method becomes completely unnecessary... it's merely checking the same
condition that all the other waitFor uses are... the only difference is that
this new method checks it inside the Wait class, and all the other uses check
it just outside... it seems kind of pointless to add a method that does the
same thing as the existing methods, but is used slightly differently. It's like
having a methods for commutative operations like: `multiplyAB(A, B)` and a
separate `multiplyBA(B, A)`. We only need the one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
