Tests expecting AssumptionViolatedException should be marked as passed, not skipped #1291

alb-i986 · 2016-05-12T23:49:50Z

Tests annotated with @Test(expected = AssumptionViolatedException.class)
which throw AssumptionViolatedException should be marked as passing, not skipped.

Fixes #1290

Tests annotated with `@Test(expected = AssumptionViolatedException.class)` which throw AssumptionViolatedException should be marked as passing, not skipped.

If the AssumptionVaiolatedException is thrown, the unit test should fail, instead it would have been marked as skipped!

QinJiangbo · 2016-05-13T04:02:23Z

😎

kcooney · 2016-05-13T05:26:59Z

src/test/java/org/junit/internal/runners/statements/ExpectExceptionTest.java

+
+    @Test
+    public void givenWeAreExpectingAssumptionViolatedExceptionAndAStatementThrowingAssumptionViolatedException() {
+        ExpectException sut = new ExpectException(new StatementThrowingAssumptionViolatedException(), AssumptionViolatedException.class);


Can we instead test this in org.junit.tests.running.methods.ExpectedTest in the same way we test the expected attributes? We get a bit more confidence if when test features like this by writing tests that run specific tests and verify JUnit gives the proper result for those test runs.

Sure I'll do that. But I also would like to keep this unit test, which is really a unit test, small.

Sure, having both kinds of tests is fine. The integration test is necessary to verify the bug is fixed; the unit test verifies the contract of the ExpectException class.

If we keep this test class, please have the class Javadoc say where the integration tests are (using@link)

A few suggested changes:

Could you update this test to use https://github.com/junit-team/junit4/blob/master/src/main/java/org/junit/internal/runners/statements/Fail.java ? That way, you can see what exception the Statement is throwing by looking at the test method.

Extract a local variable for the expression you pass to the ExpectException constructor and call that variable delegate.

I don't think we use sut as a variable name very often. If we don't, I suggest naming it expectException so future readers wont be confused by the name.

Also, if you are adding unit tests, please add a test that makes sure that tests that expect AssumptionViolatedException but do not throw it should fail

Could you update this test to use https://github.com/junit-team/junit4/blob/master/src/main/java/org/junit/internal/runners/statements/Fail.java ?

done

Extract a local variable for the expression you pass to the ExpectException constructor and call that variable delegate

Done, but I gave it a more meaningful name, although a bit longer: statementThrowingAssumptionViolatedException

I don't think we use sut as a variable name very often. If we don't, I suggest naming it expectException so future readers wont be confused by the name

Done but I would suggest to consider adopting it.
I think I took it from xunitpatterns, see e.g. http://xunitpatterns.com/Test%20Stub.html (scroll to "Motivating Example"):

public void testDisplayCurrentTime_AtMidnight() { // fixture setup TimeDisplay sut = new TimeDisplay(); // exercise sut String result = sut.getCurrentTimeAsHtmlFragment(); // verify direct output String expectedTimeString = "<span class=\"tinyBoldText\">Midnight</span>"; assertEquals( expectedTimeString, result); }

I find it a great name as it clearly stands out among the many variables (the DOCs, following Meszaros's terminology) which may be defined in a unit test.

It also makes it extremely easy to distinguish the "when" step: it's the one (and only, in theory) of the form sut.method(inputs). In the example above, sut.getCurrentTimeAsHtmlFragment();

Also, if you are adding unit tests, please add a test that makes sure that tests that expect AssumptionViolatedException but do not throw it should fail

Done. I've also added a couple more scenarios:

scenario where the given statement passes

scenario where the statement throws subclass of AssumptionViolatedEx

marcphilipp · 2016-05-14T11:47:17Z

This fix looks fine. However, I'm not sure this use case makes sense.

@alb-i986 You had a test in your codebase that used @Test(expected = AssumptionViolatedException.class)?

kcooney · 2016-05-14T17:39:01Z

src/test/java/org/junit/internal/runners/statements/ExpectExceptionTest.java

+            sut.evaluate();
+            // then no exception should be thrown
+        } catch (Throwable e) {
+            fail("should not throw anything, but was thrown: " + e);


~~No need to catch unexpected exceptions; if evaluate throws an exception then the test would fail.~~

Edit: Woops! I see why you need to do an explicit catch; without your fix this test would not fail because of the assumption failure exception being thrown.

Ya right! Tricky uh? :) Testing JUnit with JUnit is tricky itself

kcooney · 2016-05-14T17:46:59Z

@marcphilipp we have tests like this in JUnit. See #1290

alb-i986 · 2016-05-14T19:26:36Z

You had a test in your codebase that used @test(expected = AssumptionViolatedException.class)?

As Kevin said, no I don't, I've found one in JUnit itself:

https://github.com/junit-team/junit4/blob/master/src/test/java/org/junit/tests/experimental/AssumptionTest.java#L80

I share your concern in #1290, where I was commenting

But maybe this is actually working as expected. I mean, no one should ever expect AssumptionViolatedException. It can happen only to us, in our unit tests. Wherever we need to, we have to use a try/catch block instead.

However, I think the scenario may happen outside as well, although rare.
So this should make sense.

kcooney · 2016-05-15T21:44:23Z

I agree that no one should ever expect AssumptionViolatedException but the behavior they currently get if they do that may be unexpected.

This change seems safe. The tests were previously marked as skipped, and now they will be marked as passing. Then again, some users might expect that any test method with a violated assumption should be marked as skipped.

Another option is to have tests annotated with Test(expected = AssumptionViolatedException.class) fail with a message indicating that this construct is ambiguous, and directing them to use assertThrows or expectThrows.

- var name: sut -> expectException - rename test method name - class javadoc linking to the integration tests - add a test verifying that tests that expect AssumptionViolatedException but do not throw should fail - add scenario where the given statement passes - add scenario where the statement throws subclass

marcphilipp · 2016-05-19T18:30:26Z

I tend to think that we should mark them as passing.

@junit-team/junit-committers Opinions?

kcooney · 2016-05-20T01:19:18Z

src/test/java/org/junit/internal/runners/statements/ExpectExceptionTest.java

+    @Test
+    public void whenExpectingAssumptionViolatedExceptionStatementsThrowingItShouldPass() {
+        Statement statementThrowingAssumptionViolatedException = new Fail(new AssumptionViolatedException("expected"));
+        ExpectException expectException = new ExpectException(statementThrowingAssumptionViolatedException, AssumptionViolatedException.class);


If you want to keep the long variable name for the Statement arg then please wrap this line to make it readable:

ExpectException expectException = new ExpectException( statementThrowingAssumptionViolatedException, AssumptionViolatedException.class);

But I really prefer delegate or delegateStatement because that is what the role of the Statement is for the sut. The variable is declared one line up, so the long name isn't needed and makes the code harder to scan IMHO.

kcooney · 2016-05-20T01:34:40Z

Sorry for nit-picking about the tests, but I found the original unit tests to be hard to understand

kcooney · 2016-05-20T01:37:14Z

Marc, I agree that this is a good fix.

kcooney

Thanks for the changes!

I'll leave this open a bit to see if any of the other maintainers have concerns.

kcooney · 2017-02-14T02:19:22Z

Merged

@alb-i986 would you please update the release notes?

alb-i986 · 2017-02-15T00:05:59Z

@kcooney
Sure, done.
Cheers

…d, not skipped (junit-team#1291) Tests annotated with `@Test(expected = AssumptionViolatedException.class)` which throw AssumptionViolatedException should be marked as passing, not skipped. Fixes junit-team#1290

Fix junit-team#1290

c71c878

Tests annotated with `@Test(expected = AssumptionViolatedException.class)` which throw AssumptionViolatedException should be marked as passing, not skipped.

alb-i986 mentioned this pull request May 12, 2016

Tests annotated with @Test(expected = AssumptionViolatedException.class) will be marked as skipped instead of passing #1290

Closed

alb-i986 added 3 commits May 13, 2016 01:02

add unit test

939ade5

fix unit test

6def963

If the AssumptionVaiolatedException is thrown, the unit test should fail, instead it would have been marked as skipped!

fix style: rm double return line

e6aee89

alb-i986 mentioned this pull request May 13, 2016

Fix mvn surefire config: better test output #1289

Merged

kcooney reviewed May 13, 2016
View reviewed changes

add integration test

4a79e75

kcooney reviewed May 14, 2016
View reviewed changes

alb-i986 added 2 commits May 19, 2016 00:59

Merge branch 'master' into fix-1290

ca669b6

kcooney reviewed May 20, 2016
View reviewed changes

kcooney added the 4.13 label Jul 17, 2016

kcooney changed the title ~~Fix #1290~~ Tests expecting AssumptionViolatedException should be marked as passed, not skipped Nov 30, 2016

kcooney added the changes requested label Nov 30, 2016

alb-i986 added 2 commits January 24, 2017 00:33

adressing review comments

95800b6

Merge branch 'master' into fix-1290

0ea179f

kcooney self-assigned this Feb 12, 2017

kcooney approved these changes Feb 12, 2017

View reviewed changes

kcooney removed the changes requested label Feb 12, 2017

marcphilipp approved these changes Feb 12, 2017

View reviewed changes

kcooney merged commit a29f45a into junit-team:master Feb 14, 2017

kcooney added the needs release notes update label Feb 14, 2017

alb-i986 deleted the fix-1290 branch March 1, 2017 22:18

kcooney modified the milestone: 4.13 Aug 6, 2017

kcooney removed the 4.13 label Aug 6, 2017

stefanbirkner removed the needs release notes update label Oct 6, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tests expecting AssumptionViolatedException should be marked as passed, not skipped #1291

Tests expecting AssumptionViolatedException should be marked as passed, not skipped #1291

alb-i986 commented May 12, 2016 •

edited by kcooney

Loading

QinJiangbo commented May 13, 2016

kcooney May 13, 2016

alb-i986 May 13, 2016

kcooney May 14, 2016

kcooney May 14, 2016

alb-i986 May 19, 2016

marcphilipp commented May 14, 2016

kcooney May 14, 2016 •

edited

Loading

alb-i986 May 14, 2016

kcooney commented May 14, 2016

alb-i986 commented May 14, 2016

kcooney commented May 15, 2016 •

edited

Loading

marcphilipp commented May 19, 2016

kcooney May 20, 2016

kcooney commented May 20, 2016

kcooney commented May 20, 2016

kcooney left a comment

kcooney commented Feb 14, 2017

alb-i986 commented Feb 15, 2017

Tests expecting AssumptionViolatedException should be marked as passed, not skipped #1291

Tests expecting AssumptionViolatedException should be marked as passed, not skipped #1291

Conversation

alb-i986 commented May 12, 2016 • edited by kcooney Loading

QinJiangbo commented May 13, 2016

kcooney May 13, 2016

Choose a reason for hiding this comment

alb-i986 May 13, 2016

Choose a reason for hiding this comment

kcooney May 14, 2016

Choose a reason for hiding this comment

kcooney May 14, 2016

Choose a reason for hiding this comment

alb-i986 May 19, 2016

Choose a reason for hiding this comment

marcphilipp commented May 14, 2016

kcooney May 14, 2016 • edited Loading

Choose a reason for hiding this comment

alb-i986 May 14, 2016

Choose a reason for hiding this comment

kcooney commented May 14, 2016

alb-i986 commented May 14, 2016

kcooney commented May 15, 2016 • edited Loading

marcphilipp commented May 19, 2016

kcooney May 20, 2016

Choose a reason for hiding this comment

kcooney commented May 20, 2016

kcooney commented May 20, 2016

kcooney left a comment

Choose a reason for hiding this comment

kcooney commented Feb 14, 2017

alb-i986 commented Feb 15, 2017

alb-i986 commented May 12, 2016 •

edited by kcooney

Loading

kcooney May 14, 2016 •

edited

Loading

kcooney commented May 15, 2016 •

edited

Loading