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
dontCatchUncaughtExceptions by default #119
Comments
Hi, Thanks for your comment. You're the first person to say this ever in the Awaitility's 8 year history so I'm a bit hesitant on changing this. Especially since many times (I would guess most of the times?) when testing systems that are using multiple threads you want to get the error regardless of which thread threw the exception. You might not know which thread that is causing the error and you just want to fail your test any error. Also this would break backward compatibility which is also negative. WDYT? |
I am confused by the situation, that a test without using the Awaitility will be successful, but it breaks with the Awaitility. Most likely this is more actual for the process of refactoring old tests than for creating a new test. I completely agree with the fact that breaking back compatibility is bad. Maybe it will be reasonable to make an example of usage the |
Guess I'm a bit biased since I always start off by using Awaitility ;) Now I better understand what your concern is. I would still suspect that it's more common that you find the current default to be useful otherwise I think someone should have brought this up before.
I'm all for improving the docs! If you have an example of what you would like to see I can add it to the docs. |
I tried to make a commit in the documentation, but I didn't found a way to make a PR in your github wiki. I put my thoughts here: Exception handlingBy default Awaitility catches uncaught throwables in all threads and propagates them to the awaiting thread. This means that your test-case will indicate failure even if it's not the test-thread that threw the uncaught exception. You can choose to ignore certain exceptions or throwables, see here. If you don't need to catch exceptions in all threads you can use dontcatchuncaught(link to example 12.). Example 12 - Ignoring uncaught exceptions:If you migrate your code from using the You can turn off it by using the @Test
public void dontCatchUncaughtExample() {
ThreadPoolTaskExecutor executor = new ThreadPoolTaskExecutor();
executor.setMaxPoolSize(1);
executor.afterPropertiesSet();
executor.execute(new ErrorTestTask());
ListenableFuture<?> future = executor.submitListenable(new ErrorTestTask());
Awaitility.await()
.dontCatchUncaughtExceptions()
.atMost(1, TimeUnit.SECONDS)
.pollInterval(10, TimeUnit.MILLISECONDS)
.until(future::isDone);
}
private static class ErrorTestTask implements Runnable {
@Override
public void run() {
try {
Thread.sleep(100);
} catch (InterruptedException e) {
e.printStackTrace();
}
throw new RuntimeException(Thread.currentThread().getName() + " -> Error");
}
} |
I made some changes (hope you don't mind) and added it to the docs now. Have a look and tell me what you think. |
Thanks for your work! You made a very useful utility, good luck with your projects! |
Thank you for the kind words and for the contribution. |
I discovered this issue from @antkorwin's blog post and it was indeed a very surprising behaviour. P.S. now I'm checking many projects I have (since I love Awailitity and use it in many places) and thinking whether I should explicitly disable this behaviour there, especially when the target system (e.g. a Spring Boot app) runs in the same JVM as the tests' (integration tests, for instance) |
@bsideup Thanks for bringing this up again. I'm working on Awaitility 4.0 now so if we want to introduce a breaking change now is a good time. Maybe you could elaborate to me a bit more why From what I can recall from @antkorwin's blog he had tests that used Maybe this is a bit naive (sorry), but when I look at the example above I more or less see something similar to this: public class SomeClass {
public boolean isDone() {
try {
Thread.sleep(100);
} catch (InterruptedException e) {
e.printStackTrace();
}
throw new RuntimeException("Error");
}
} And if I write a test like this: SomeClass someClass = new SomeClass();
assertThat(someClass.isDone()).isTrue(); I would except it to fail. So with this reasoning, why wouldn't the same behavior apply if this happens in a different thread. |
I have a counter argument to my own example though. Reading the Javadoc of
With this in mind I better understand the argument, at least for Future f = ..
await().untilDone(f); where in this case Awaitility wouldn't catch uncaught exceptions (since we're dealing with futures). But maybe this will make things even more complex? |
Awaitility 4.0 - it's good news!
Before change something it would be good to consider both sides. Can you show a real example of a test where the default behavior is more expected than "dontCatchUncaughtExceptions"? I think the main reason for this problem is unexpected behavior for a few cases (migration from Thread.sleep to Awaitility is a real example of this). But if we have only one example of a mismatch between the expected behavior and default behavior, and have a lot of cases where this is more expected then it's quite logical to leave all as it is. Besides, if we change the behavior now, then we break the compatibility, especially if there are many examples of using the default behavior.
In my opinion, the making of another kind of the asserting for Futures it's not necessary, if you caught this problem and went to documentation then you already found a note about this behavior (and you know how to use uncaught option). I think we have a good chance that a special API for Futures will not be used. |
Unfortunately I cannot show an actual example but if I remember it correctly I've been "saved" by it a few times when doing larger integration/acceptance tests. For example if you have a process manager and you kick of a process by sending a command, C, and at the end of the process something should have happened (E). During the life-cycle of this process (C to E) there might be a lot of things happening in different threads and one of these threads might throw an unexpected exception. This has happened to me in the past and this is the reason why "catch all uncaught exceptions" is true by default. I admit that it's happened like 2-3 times at most for me in the 9 year history of Awaitility. You could of course argue that you should have caught these exceptions in other tests, and I agree, but it wasn't until I wrote these larger integration tests where the threading problems became prominent since the entire environment was up and running for a larger use case. It's hard to know which is the best default option since obviously no one that's positive of having "catch all uncaught exceptions" as default has complained yet :) But I could very well image that your use case might be more frequent, i.e. that you migrate from tests having WDYT? |
@antkorwin @bsideup Any comments? This is the main concern I have before releasing 4.0 and I'd like to proceed :) |
I think it's worth mentioning in the doc that it's not just all threads, it's all threads without |
What do you think about making the
dontCatchUncaughtExceptions
a setting by default?I think, sometimes catchs of uncaught exceptions lead to unexpected behaviour in tests,
for example:
When I replaced Thread.sleep on the Awaitility, I got a failed test.
I encountered this problem when I refactored a lot of tests to use the Awaitility instead of the Thread.sleep. It took quite a long time to understand that the problem was in another thread.
The text was updated successfully, but these errors were encountered: