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

Unwrap InvocationTargetException thrown by QuarkusTest*Callback #23989

Merged
merged 1 commit into from
Feb 28, 2022
Merged

Unwrap InvocationTargetException thrown by QuarkusTest*Callback #23989

merged 1 commit into from
Feb 28, 2022

Conversation

famod
Copy link
Member

@famod famod commented Feb 26, 2022

Improves readability when you have e.g. a custom callback that validates something.

Some methods are only allowed to throw Exeption, not Throwable, hence the cause type check.

Map.Entry<Class<?>, ?> tuple = createQuarkusTestMethodContextTuple(context);
afterEachCallback.getClass().getMethod("afterEach", tuple.getKey())
.invoke(afterEachCallback, tuple.getValue());
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running this outside of the TCCL restoration block looked wrong, so I moved it inside.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm... Not sure about this... Did you see an issue with the existing code?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not an actual one, but compare it with this:
https://github.com/quarkusio/quarkus/blob/2.7.2.Final/test-framework/junit5/src/main/java/io/quarkus/test/junit/QuarkusTestExtension.java#L407

Why should the TCCL only be restored if there is an exception in a beforeEachCallback, but not if there is an exception in an afterEachCallback? Doesn't make sense to me.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PS: afterAll also restores the TCCL just like beforeEach. Same happens for beforeClassCallback. 🤷‍♂️

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change looks right to me

@famod
Copy link
Member Author

famod commented Feb 26, 2022

I'll add the backport label because I think that TCCL thing is a bug (rare though).

@stuartwdouglas stuartwdouglas merged commit 4e56d41 into quarkusio:main Feb 28, 2022
@quarkus-bot quarkus-bot bot added this to the 2.8 - main milestone Feb 28, 2022
@gsmet gsmet modified the milestones: 2.8 - main, 2.7.3.Final Feb 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants