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

Don't convert assumption failures to errors in ErrorCollector.addError() #1703

Open
prbprbprb opened this issue Mar 2, 2021 · 1 comment

Comments

@prbprbprb
Copy link

prbprbprb commented Mar 2, 2021

This was previously discussed in #1363 and I think the new treatment of ErrorCollector. checkSucceeds() is correct. However ErrorCollector.addError() is more commonly used when calling sub-methods or in wrapper Rules (AOSP example) so it seems clearer (to me) to pass assumption failures through.

#1702 is potential fix, although this has similar properties to what #1363 complained about, i.e. if addError() is used to add multiple assumption failures then they'll get re-thrown as a MultipleFailureException causing the test to fail. It seems to me that in that case the test probably needs restructuring anyway (e.g. to check its assumptions before calling multiple sub-methods), but I'm open to opinions.

@stefanbirkner
Copy link
Contributor

stefanbirkner commented Jul 7, 2021

The code works as expected. From @kcooney's remarks in #1363

The expressions passed into these methods should call production code, not test code, and AssumptionViolationException should generally be thrown by test code.

The Javadoc can be improved but currently we haven't planned to build another JUnit 4 release.

Looking at the provided example I think it should be changed to

  ...
} catch (AssumptionViolatedException e) {
  throw e;
} catch (Throwable e) {
  mPostAssertionRule.addError(e);
} finally {
  ...

so that it immediately stops the test. @prbprbprb what do you think?

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

No branches or pull requests

2 participants