ParentRunner does not document itself as calling collectInitializationErrors in it's constructor.

[eatnumber1]

ParentRunner and it's subclasses should document the methods which are meant to be overridden, but are transitively called from ParentRunner's constructor.

Arguably, this is a design flaw in ParentRunner, as with the current behavior, subclasses which override these methods will be called before those subclasses begin initialization which severely limits the usefulness of overriding these methods.

[ashleyfrieze]

Looks like the method should be final, then?

[eatnumber1]

Yes, agreed. It will break some existing code though.

[ashleyfrieze]

This is an accident waiting to happen. What if the call to collectInitialisationErrors were made from either the getChildren or getDescription methods instead? This practically wouldn't change the order of events, but would avoid the "call overridable from constructor" antipattern.

[kcooney]

I wonder if we can fix this by adding an Initializable interface (similar to Sortable) and have RunnerBuilder check if the runner is Initializable and if so call theinitialize() method.

[ashleyfrieze]

I think that approach would break those subclasses of existing runners which do not get built through the RunnerBuilder.

Though the change could alter this, org.junit.runners.parameterized.BlockJUnit4ClassRunnerWithParametersFactory is an example of a class which wouldn't initialize correctly after this change. A lot of 3rd party subclasses of Block would break in a similar way.

I guess this is the opposite of the side effects from making that collectInitializationErrors and the functions called by it final.

[kcooney]

@ashleyfrieze while you do have a point that some code creates runners without going through RunnderBuilder, the only thing what would break is that we wouldn't do some of the validation checks.

Changing extendable methods to final methods will break an unknown number of people in a much more visible way. It would also only solve the symptom, not the problem; the next time we need some early validation, we would hit the same problem.

Doing the validation in getChildren() changes a method that should only fail in extreme cases to one that can fail if a validation check fails (which can happen if you forget to make a field or method public). We would need to ensure that callers handle the failures gracefully. The RunnerBuilder code already handles failures.

@junit-team/junit-committers any thoughts as to which way is best to resolve this problem?

[dsaff]

We made an early decision that it should not be possible to ever get a reference to a Runner object that couldn't be validly configured. So if a Runner won't be able to run as requested, it must throw an exception from its constructor.

If I had it to do over again, I think we would do things differently, but I don't think we can undo that decision before some kind of "JUnit 5"-sized overhaul.

Can you give an example of something you'd want to do in a ParentRunner subclass that you're currently finding difficult? It may be that we can add documentation to direct people to effective workarounds.

[eatnumber1]

My difficulties start with the inability to define custom constructors on runners.

I want to create test classes using Guice injection. This means that the all the runners involved need to pass around a single Injector instance (therefore all the involved runners will have shared state). Because I cannot pass the injector into sub-runners (even when using a RunnerBuilder) via it's constructor, I'm forced to use a setter. This setter is called after the validation checks, and therefore I'm not able to perform validation on the Injector.

Furthermore, even if all the state was contained within the Runner itself, I would not be able to access e.x. private fields in my subclass of ParentRunner during validation, as they will have not yet been initialized.

Even non-final inline initialized fields won't be initialized yet.

[dsaff]

Can you do the validation you want in the RunnerBuilder, or in the setter?

I know that there are several categories of things that the current implementation makes difficult, and want to be sure we're prioritizing the things that are causing the most pain.

[eatnumber1]

I've actually worked around the issue by making what I call a DelegatingRunner. It's a class which implements Runner by lazily constructing the actual Runner when any of Runner's methods are called. I then subclass DelegatingRunner and return a private impl class when DelegatingRunner asks to construct the actual runner. This allows me to have a custom constructor.

Keep in mind that although this bug is pointing out the design flaw, I'm not asking you to fix it immediately; it's hard to fix properly. I'm just asking you to document it!