Spectrum 1.1.0 displays test case nested structure wrong in Intellij Idea

[anttiahonen]

Unfortunately version 1.1.0 seems to have introduced funky behavior in Intellij Idea test output structure in case of exception inside tests.

First the test code that throws exception:

Second how Intellij Idea displays it with Spectrum 1.0.0

Third the same test with Spectrum 1.1.0

Happens at least with Intellij Idea 2017.1 and 2016.3.5

[ashleyfrieze]

This looks to be an issue with the way that Spectrum informs the RunNotifier about failure and where that happens in the process. The RunNotifier can be very picky about what you say to it and when. Looks like the more exotic hook mechanism we (and I mean I) added has some unexpected side effects when there are errors in the pre-test hooks.

@greghaskins do you mind if I investigate this over the next few days. Have any ideas of your own?

[greghaskins]

@ashleyfrieze be my guest. Thanks for the help. I don't have any ideas off hand, but interested to see what you find.

[ashleyfrieze]

I think it may also be related to #109 - I think we need to call fireTestStarted for a failing test to explain why it failed. My guess is that the solution to this may lay in abstracting away from RunNotifier, which can only help move toward test framework agnosticism.

I did a little spike, and it looks like it will need more than a one line fix :|

[ashleyfrieze]

We're violating the contract with RunNotifier in a couple of ways. The hook mechanism risks getting very tangled if we didn't introduce an adapter.

I'm working on a branch here - https://github.com/ashleyfrieze/spectrum/tree/IntelliJErrorBug

Some of the problem is dealt with by 130b148

Please drop some comments either into my commit or on here. It's very WIP code. The change doesn't work yet, but includes my idea of bringing the start/finish test mechanism to become the first hook in the chain of hooks. That's fine, but results in a problem when an exception is thrown through multiple hooks, since the hooks ALL report it.

Perhaps this outer hook should report it for everyone, or perhaps something else.

I'm already looking at the idea of making the RunNotifier the subject of an adapter for when we do parallel testing, since JUnit runners will explode if you give them live test results out of order when running tests in parallel.

More to do on this. Ideas gratefully considered. Under the circumstances, I think another release would be in order. :(

I'll also work out how to get a test that needs this to be right to pass.

[ashleyfrieze]

@anttiahonen if you would like to pull the branch behind #111 and try it on both of the issues you raised, I'd appreciate the feedback. I had reproduced your problem with this test:

https://github.com/ashleyfrieze/spectrum/blob/IntelliJErrorBug/src/test/java/specs/BeforeEachSpecs.java

That test didn't make it into my solution, though, since I produced a proxy for it which focused more on the root cause - the violation of the interface of RunNotifier.

I think we're still deliberately violating that interface by pushing several failures to it relating to the same test, but perhaps that's not going to cause you any noticeable issues.

[greghaskins]

@ashleyfrieze Unfortunately, this doesn't seem to be fully resolved after #111. I did a quick check by picking a spec at random and making it throw an exception to see how IntelliJ reports it. Looks like weird things are getting reported when something goes wrong in beforeAll:

For comparison, with 1.0.2, the reporting seems to do the right thing:

Update: it looks like this only affects beforeAll and aroundAll (when an exception occurs before running the suite block). Here's what happens with aroundAll:

[ashleyfrieze]

Looks like someone is going to have to fix it some more. While I'm there I'll look at how JUnit mixins' @BeforeAll also works.

[greghaskins]

Closed by #112