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

Add overload of TestWatcher.skipped() that uses the external AssumptionViolationException #928

Conversation

kcooney
Copy link
Member

@kcooney kcooney commented Jun 8, 2014

Add overload of TestWatcher.skipped() that uses the external AssumptionViolationException

This allows code using TestWatcher to handle assumption violation exceptions
without using deprecated classes.

@dhasday
Copy link
Contributor

dhasday commented Jun 9, 2014

With these updates, shouldn't there also be some tests that use the external version of AssumptionViolatedException to exercise the functionality? It looks like TestWatcherTest is only using the internal version.

@kcooney
Copy link
Member Author

kcooney commented Jun 9, 2014

@dhasday Thanks for taking a look The methods on Assume throw the external version of AssumptionViolatedException

@dhasday
Copy link
Contributor

dhasday commented Jun 9, 2014

@kcooney Sure, that makes sense, but these tests only verify backwards compatibility since all of the overridden skipped methods are using the internal version of AssumptionViolatedException. There doesn't seem to be any unit test coverage to verify an overridden skipped with the external version, specifically that it does not call skipped with the internal version too.

@kcooney
Copy link
Member Author

kcooney commented Jun 9, 2014

@dhasday LoggingTestWatcher.skipped() uses the external version of AssumptionViolatedException. I don't expect people to write subclasses of TestWatcher where they override both versions of skipped() so I don't feel the need to test for test cases related to that.

If someone explicitly throws the internal AssumptionViolatedException in a test with a TestWatcher that only overrides the version of skipped() that uses the external AssumptionViolatedException then the skipped method won't be called (the external version extends the internal version). I didn't write a test for that, because I don't generally write tests to test that something didn't happen unless that something shouldn't happen. Is that the test case you mean?

@dhasday
Copy link
Contributor

dhasday commented Jun 9, 2014

@kcooney I was specifically referring to the logic in skippedQuietly() that determines which version of skipped() to call since it's explicitly choosing between the internal and external versions.

As to LoggingTestWatcher, I was mistaken about that usage. Yes, it definitely covers the case where skipped() with the external version is used.

@@ -29,12 +29,46 @@ public void succeeds() {
}
}

public static class InternalViolatedAssumptionTest {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you move this class down after neitherLogSuccessNorFailedForViolatedAssumption The internal classes are always positioned above the test that is using them.

Copy link
Member Author

Choose a reason for hiding this comment

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

@stefanbirkner Done and rebased and squashed commits.

…onViolationException.

This allows code using TestWatcher to handle assumption violation exceptions
without using deprecated classes.
@stefanbirkner stefanbirkner added this to the 4.12 milestone Jun 23, 2014
stefanbirkner added a commit that referenced this pull request Jun 23, 2014
…-violation

Add overload of TestWatcher.skipped() that uses the external AssumptionViolationException
@stefanbirkner stefanbirkner merged commit e1315c7 into junit-team:master Jun 23, 2014
@stefanbirkner
Copy link
Contributor

Thanks.

@kcooney kcooney deleted the test-watcher-internal-assumption-violation branch June 29, 2014 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants