AssumptionViolatedException and TestAbortedException are treated incorrectly

[yeugenius]

If JUnit's AssumptionViolatedException was thrown inside the test, Spock treats the test as ignored, but report will show the test as executed.

AssumptionViolatedException is a useful feature to skip tests arbitrary during the Spec execution based on input data and logic that is not available before Spec starts and so far not possible t use IgnoreIf annotation.

[renatoathaydes]

Oh, thanks for reporting! Will have a look into it.

[renatoathaydes]

Spock is not reporting that the feature was not executed. It doesn't seem to give any information at all indicating that the test did not run.

I will continue trying to figure this out.

[renatoathaydes]

As far as I can tell, it is currently impossible to know that the test did not execute.

Spock code calls only org.junit.runner.notification.RunNotifier#fireTestAssumptionFailed but there's no way I can see to add a JUnit Listener to the Spock JUnit Runner. If you know a way to achieve that, please let me know.

[renatoathaydes]

The problematic code is in org.spockframework.runtime.JUnitSupervisor:

    if (exception instanceof AssumptionViolatedException) {
      // Spock has no concept of "violated assumption", so we don't notify Spock listeners
      // do notify JUnit listeners unless it's a data-driven iteration that's reported as one feature
      if (currentIteration == null || !currentFeature.isParameterized() || currentFeature.isReportIterations()) {
        notifier.fireTestAssumptionFailed(failure);
      }
    }

https://github.com/spockframework/spock/blob/master/spock-core/src/main/java/org/spockframework/runtime/JUnitSupervisor.java#L92

To fix this issue, Spock itself has to start calling the Spock listeners, not only JUnit's.

At least, that's my current understanding. Maybe @pniederw could help solve this (or add a method to the Spock listeners to listen for this).

[yeugenius]

IDEA reports in test tab AssumptionViolatedException tests as 'Test ignord'. It looks pretty logical. That's why I was surprised when I saw ignored tests in IDE as executed and passed in spock html report.

Maybe Spock can just call:

notifier.fireTestIgnored("Ignored due to AssumptionViolatedException" + feature.getDescription())

[viastakhov]

@yeugenius, @renatoathaydes
Seems like there aren't any opened issues in spock regarding to this defect.
Could you update the status of the issue, or open new one in spock in order to proceed investigation of the defect?
Thanks.

[renatoathaydes]

I would make a pull request against Spock to make sure it does what @yeugenius suggested, but I am very short on spare time right now... hope someone else can have a look at that.

[renatoathaydes]

I've come up with a workaround for this issue.
I added a new extension function to Specification so that instead of explicitly throwing an AssumptionViolatedException from your tests, you can do this instead:

    @Unroll
    def "Can ignore test if AssumptionViolationException is thrown"() {
        setup: 'this will throw AssumptionViolationException if the condition is false'
        assertTestPrecondition( 'i should not be 2!', i != 2 )

        expect:
        i > 0

        where:
        i << [ 1, 2, 3 ]
    }

The assertTestPrecondition method will just throw AssumptionViolatedException with the message given, but it will also let spock-reports know the test is being skipped.

Would that work for you @yeugenius @viastakhov @SamuelGraf ?

[renatoathaydes]

Ah, nevermind, spock-reports is just a report creator... I don't want to introduce unrelated APIs. My next best suggestion is to capture invocations of the AssumptionViolatedException constructor and assume that if one is created while a spec is running, then it must have been skipped?!

The drawbacks are:

• if something catches the exception, then we'll incorrectly think the spec was skipped.
• only works in Groovy code. Throwing it from Java classes won't be detected.

[SamuelGraf]

Of course this is not optimal but I think it would be much better as it is now (at least for me).
to 1) I think it is better to mark a passed test as skipped as vice versa. I think it is much worse to miss a bug than to believe in a bug to exist, that does not exist.
Would it be possible to somehow explicitly tell the report in the catch-block, that the exception was caught?
to 2) I mean better in Groovy than in nothing.

I wonder how IntelliJ and the gradle report deal with this because they seem to get it right (at least from my experience). Unfortunately I am a beginner and don't understand much of this.

[renatoathaydes]

The tools that work, only work because they integrate with JUnit, not directly with Spock.

spock-reports integrates directly with Spock, not with JUnit, and because Spock does not notify spec listeners that the test was skipped, there's no good way of knowing that.

The ideal solution is to make a PR against the Spock project as I mentioned above.

[SamuelGraf]

So I should change:

if (exception instanceof AssumptionViolatedException) {
      // Spock has no concept of "violated assumption", so we don't notify Spock listeners
      // do notify JUnit listeners unless it's a data-driven iteration that's reported as one feature
      if (currentIteration == null || !currentFeature.isParameterized() || currentFeature.isReportIterations()) {
        notifier.fireTestAssumptionFailed(failure);
      }
}

to

if (exception instanceof AssumptionViolatedException) {
      notifier.fireTestIgnored("Ignored due to AssumptionViolatedException" + feature.getDescription())
      // do notify JUnit listeners unless it's a data-driven iteration that's reported as one feature
      if (currentIteration == null || !currentFeature.isParameterized() || currentFeature.isReportIterations()) {
        notifier.fireTestAssumptionFailed(failure);
      }
}

and make a merge request with spock? As I said, I am a beginner and not quite sure about what i am doing here.

[renatoathaydes]

I thought of submitting a PR doing this, but it was not clear to me how to handle test iterations.

Your change seems incorrect for two reasons:

• you would probably need to notify the Spock listeners only in the case where you also notify the JUnit notifier, i.e. inside the if block.
• you are calling notifier.fireTestIgnored when you should be calling masterListener.iterationSkipped(IterationInfo iteration), which does not yet exist.

There's only a void featureSkipped(FeatureInfo feature); method in the Spock listener, but this bug is not about skipping the full feature, just an iteration... so first of all, Spock would need to add the masterListener.iterationSkipped(iteration) method for us to be able to call it.

This is more a feature request than a bug fix on the Spock side.

[SamuelGraf]

I see,
can you do the feature request, since I think you know better what exactly is needed. Otherwise I would try to explain what is need as far as I understood and link this comment and/or issue.

[renatoathaydes]

@SamuelGraf thanks for pushing for a fix for this :) I've opened a feature request with Spock... I know they are very careful with introducing new features, so let's see what they think about it before we proceed.

[SamuelGraf]

Sorry, if I am too pushy,
It just would be awesome, so I try to make it work.
Thx for opening spockframework/spock#1008

[renatoathaydes]

As far as I can tell, in Spock 2.0, throw new AssumptionViolatedException('') causes the test to fail. Both the Gradle report as well as spock-reports consider this a test failure... that's because this Exception is part of JUnit4 but Spock 2 uses JUnit 5 (see spockframework/spock#1185).

Unfortunately, though, when the equivalent JUnit5 TestAbortedException is thrown from a test iteration, Spock still fails to notify extensions (including spock-reports, obviously), and hence this is still a bug.