Add timeout pre-condition

[ashleyfrieze]

Similar to JUnit's @Test(timeout=1234) this is a suggestion to extend the current pre-condition functionality a bit like this:

it("will run but not for long", with(timeout(1234), ()->{
   // spec code
}));

This is quite easy to do, since the PreConditionBlock can just handle it for us, wrapping the real block in a timing out sentinel. But there's a wrinkle. What does:

describe("this has a timeout so what does it mean?", with(timeout(1234), ()-> {
   // specs and describes go in here
   it("tests", () -> {});
}));

If we think it means that the whole parent block runs within a timeout, then the behaviour would happen for free. I think that's the least valuable and least expected answer. More likely it means that the above is the equivalent of:

// any time out put in a describe is sent down to its next most atomic children and not executed at parent level
describe("this has a timeout so what does it mean?", ()-> {
   // specs and describes go in here
   it("tests",  with(timeout(1234, () -> {}));
}));

Thoughts on:

• Is this valuable as a feature (clue: yes :) )
• How do we think it should pass timeouts around the tree?
• Is the above syntax and re-use of the PreConditions a good approach?

[richdouglasevans]

Is this valuable as a feature?

Yes.

How do we think it should pass timeouts around the tree?

Dunno; having a play now.

Is the above syntax and re-use of the PreConditions a good approach?

Sounds good.

If we think it means that the whole parent block runs within a timeout...

That would be surprising. Your example where such preconditions are defaults that are applied to atomic children makes intuitive sense to me.

[ashleyfrieze]

Thanks Rich. If you fancy a play look at the hooks mechanism. This will work well for describe but poorly for it.

I was wondering whether a hook for single its might be in order.

One easy way around this would be to say that timeout can only be applied to its.

So it("has a timeout", with(timeout(1234), ()->{}))); is valid and describe("suite", with(timeout(1234... would produce an error.

I don't think that's expected!

[ashleyfrieze]

Looking at this simply, if timeout were a precondition - i.e. a part of BlockConfiguration that was metadata like tags is, then we could create a simple hook which always applied to all children in spectrum and, when there was a timeout, ran the particular child in a thread with a sentinel.

So, expand BlockConfiguration and make a timeoutHook. However...

Oh boy does this violate the open-closed principle! Because if we came up with another, similar, requirement, we'd have to make two things again. Every time.

Ignoring/focusing/tags - perhaps they're special cases. But perhaps other bits of metadata need to be general, or perhaps we bring hooks closer to the action here - have a kind of pre-condition-hook thing where you can propagate a hook down the tree in a way that children can redefine...

[ashleyfrieze]

@greghaskins at the moment this timeout implementation only affects the innards of the test and not the before, around, after parts. This is in part an effect of the spectrum error handling, which goes haywire if you stop a test...

The hooks for timeout are within the error handling not downstream of it. So the aborted test cannot report errors itself on a separate thread after the test has already failed.

[ashleyfrieze]

Questions from Greg

What if the slowdown was in a beforeEach /afterEach block? I would expect this looks the same as if the test body itself was slow.

Actually, no. This question made me ponder whether I made the right decision. JUnit's timeout applies to the entirety of the test execution including @Before and @After. The timeout feature implemented in #119 ONLY covers the execution within the it block. Specifically, it applies at leaf level, not even at Atomic test level.

This could be changed.

Where does the time for beforeAll/ afterAll count? For example, if my timeout is 5 seconds and beforeAll takes 7 seconds, what happens? Then, what about multiple and nested beforeAll/ afterAll blocks?

Again - it doesn't.

Do aroundEach and aroundAll work with this? I'm guessing yes.

Not applicable.

Does the timeout hook interrupt long-running tests, or just fail if they go too long?

The hook interrupts the test execution by killing its thread. The hook, at the moment, is buried well within the execution of a low-level Spec object. This is how it gets to control what goes on, and avoid situations where the monitoring thread confuses test reporting by reporting exceptions after the test has failed, but before the thread has managed to die... (actually found the error WHILE the thread was dying).

Given aroundAll and beforeAll, can we really expect the timeout to apply to them, especially if specified down in the configuration of an it block?

In some ways, this limited implementation is easy to predict. In other ways, it may violate the POLS.

Feel free to express some pseudocode for tests that reveal your own expected behaviour and I'll either add them to the code to show it already does it, or even try to get them to pass. I have some complexish ideas for how to solve the issue of not letting the test report errors after it's timed out... the cure may be worse than the disease!

[greghaskins]

My gut feels like time spent in beforeEach and afterEach should count towards the test-level timeout (like JUnit). However, I would really prefer to match any precedent set by other tools (if there is a consistent precedent, at least). It sounds like we know the story for JUnit – what about RSpec, Jasmine/Mocha, Cucumber, etc.?

[ashleyfrieze]

Cucumber doesn't natively do timeouts to the best of my knowledge. RSpec and Jasmine don't have a test timeout feature of their own.

I think the right thing to do here is extend the test timeout to echo JUnit's rules - i.e. the whole before and after ecosystem, and possibly provide a localised timeout wrapper so people can put a timeout on a bit of their test.

E.g.

it("runs ok", () -> {
   doSomething();

   withTimeout(ofSeconds(10), () -> {
       doSomethingElse();
   });
});