-
Notifications
You must be signed in to change notification settings - Fork 149
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
throw IllegalStateException if methodInvoker is Fail #144
base: master
Are you sure you want to change the base?
Conversation
Hello, thank you for your PR! Can you describe what was a reason of the problem? Maybe we can suggest something to user in exception message because Is it possible to write test for it? Just to make sure that problem will be solved? |
Hello. I've faced something similar to what was addressed in this issue. Will get exact reasons and versions and give more insights. And yes, will try to write test and overall improve the PR. Thanks for response. |
I have experienced the same issue. If I have a parametrized test that is set up in the constructor and the constructor throws an exception @RunWith(JUnitParamsRunner.class)
public class FailingTest {
public FailingTest() {
throw new RuntimeException();
}
@Test
public void aTest() {}
} then the fail message is not accurate and the actual exception is swallowed (no stack trace available):
The issue is especially annoying when you have only parametrized test cases in your test class because you will see no exception thrown from a constructor unless you have at least one regular (parametless) test case (in that case the exception can be seen in test case initialization). |
try { | ||
methodInvoker.evaluate(); | ||
} catch (Throwable throwable) { | ||
throw new IllegalStateException("Instantiation failed", throwable); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think calling methodInvoker.evaluate()
would be enough for the fix. Ther's no need to catch the evaluation result and wraping up the original cause in IllegalStateException
. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason for that is that evaluate
is throwing Throwable
and thus method findParametr...
will not compile.
@t-izbassar Do you have plans to complete the PR by adding a missing test case for the fix (as suggested by the maintainers)? |
@goostleek would like to do it but I was caught in trying to get better solution and for testing purposes there are no specific test cases for this class, so eventually I've switched to other things) but if someone like to finish it I have no problem with that. Will try to return this PR to my to-do list, but there are no timing guarantees) |
Added throwing illegalStateException as if it is not added then this error is masked by RuntimeException with message "Cannot find invoker for the parameterised method. Using wrong JUnit version?" which might not be the case for test methods in which there are some instantiation problem.
As a client of this I was forced to debug library to see what happens. So here is PR.