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

Feature/ignoreable exceptions #27

Closed

Conversation

tkrueger
Copy link
Contributor

This adds option ignoreExceptions, which treats Exceptions that occur during condition evaluation as non-fatal, evaluating to false. This helped us cover a use case where we did RestAssured statements, which threw exceptions during the period asserts on the path failed, before settling into boolean values.

Thorsten Krüger added 3 commits July 20, 2015 14:50
Sometimes, the evaluation of the condition or its subject throw an
exception for a time before settling down. In these cases, setting
await().ignoreExceptions(true) allows to ignore these. This effectively
stops catchUncaughtExceptions() from having any effect when
ignoreExceptions() is set. No other behavior is changed.
@johanhaleby
Copy link
Collaborator

Thanks for the pull request. I will look into it soon. Just some thoughts in the mean time. I'm a bit hesitant to add this into the framework since it does add additional complexity. A workaround would be to catch the exception thrown by REST Assured (or whatever framework you're using in the condition) and return false yourself. This way this feature would not be needed in Awaitility itself. What do you think about that?

@johanhaleby
Copy link
Collaborator

But perhaps the tests will be more readable with this feature so that it's worth it?

@johanhaleby
Copy link
Collaborator

If I understand it correctly then in Java 8 you could just create something like this:

public static Callable<Boolean> ignoreIfException(Runnable runnable) {
        return () -> {
            try {
                runnable.run();
            } catch (Exception exception) {
               return false;
            }
            return true;
        };
}

And use it like this:

await().until(ignoreIfException(() -> get("/x").then().assertThat().statusCode(200)));

To achieve the same result? Please correct me if I'm wrong

@tkrueger
Copy link
Contributor Author

Yes, your example has roughly the same effect. What we did is to add a try/catch in the AwaitilityGroovyBridge. This worked fine, except when using an await() in a @BeforeClass method, where the exception slipped through. Placing the code into the framework did help us with this, but I think using the wrapper from your example should cover that case, too.

Yet I still feel it would be intuitive to have a switch in a central place to treat exceptions as evaluating to false. In the end, it's the same try/catch, only embedded right where the condition is evaluated, and used just like the other features.

Maybe it would feel less complex if it were named something like "treatExceptionsAsFalse"? I'm just thinking that writing

await().with().treatExceptionsAsFalse().until(this::contentIsPresent);

or even

Awaitility.treatExceptionsAsFalseByDefault();
// ...
await().until(this::contentIsPresent);

might have a more stringent feel to it, from the user's point of view, than using a wrapper around conditions in their own code. But I won't insist ;-)

@johanhaleby
Copy link
Collaborator

I've had your pull request in the back of my head a for long time now and I've finally come to the conclusion that it makes sense. Since you filled your pull request I've actually twice ran into a scenario where this would have helped. So I'm bringing it in but I may do some changes (for example allow to specify exactly which exceptions to ignore). Thanks a lot for your pull request and for hanging in there.

@johanhaleby
Copy link
Collaborator

I've made some changes (see changelog). Please try it out and tell me what you think. I've released a new snapshot, 1.6.5-SNAPSHOT that you can use if you add the following maven repo:

<repositories>
        <repository>
            <id>sonatype</id>
            <url>https://oss.sonatype.org/content/repositories/snapshots/</url>
            <snapshots />
        </repository>
</repositories>

@tkrueger
Copy link
Contributor Author

Cool! Will take it for a spin tomorrow.

@johanhaleby
Copy link
Collaborator

Would be great if you could try it asap, I'd like to make a new release.

@tkrueger
Copy link
Contributor Author

Tested the snapshot version with a couple of our use cases. Works like a charm. The changes you made look good, too. Thanks for bringing it in and sorry for the holdup!

@johanhaleby
Copy link
Collaborator

Thanks for helping out!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants