Clarify if TEST descriptors are allowed to have children

[octylFractal]

While working on testing custom test engines with Gradle, we discovered that it's not clearly documented if a Type.TEST descriptor is allowed to have children or not, including dynamically added ones.

My opinion on this is that they are not allowed to have children, as that is the whole point of having the container Types. As such, in Gradle, we are planning to forbid this behavior to avoid needing to delay decisions on if a node is really a container or not.

Deliverables

• Describe in TestDescriptor#getChildren Javadoc that it must be empty if the descriptor is !isContainer().
• Describe in TestDescriptor#mayRegisterTests Javadoc that it must return false if the descriptor is !isContainer().
• (optional) Enforce this in TestDescriptor#containsTests, and other places that may rely on this assumption.

[mpkorstanje]

This proposal sounds sensible. Thinking out out loud, pending team discussion:

• Are test descriptors allowed to change their type? Probably not. But currently that is not documented either.
• Can a test descriptor reject children if it is not a container? That would make the incorrect state unrepresentable and direct the problem to test engine authors.
• What are tests and container anyway? The terms are used, but as far as I can see not defined.
• Are test descriptors without children allowed to be containers? They're pruned off, but I'm not sure if that was done by the Engine or the Platform (check).

As such, in Gradle, we are planning to forbid this behavior to avoid needing to delay decisions on if a node is really a container or not.

For context, can you clarify why this is important? And does this behaviour happen with "real" test engines or only with test engines written for the purpose of testing Gradle?

[octylFractal]

It happened while we were developing our own test engine for testing Gradle, but I could see this mistake occurring to anyone that happens to develop a custom test engine.

Forbidding the behavior is required as we determine immutable information about if a test is a container or not when it's started. If a non-container suddenly reports a child test, we can't record that in our current data structures, so it would fail with an opaque error. We've decided for now that instead of doing further refactoring, adding a proper error for this would be better. The more detailed error should help direct issues towards the test engine author rather than Gradle.

What are tests and container anyway? The terms are used, but as far as I can see not defined.

I believe these are defined at https://docs.junit.org/current/user-guide/#writing-tests-definitions. A container is defined as "a node in the test tree that contains other containers or tests as its children (e.g. a test class)." This implies that a test doesn't do this, but it's not explicitly stated.

[kaipe]

I'd like to share that all types of hierarchical test-descriptors support dynamicly added children. Also I have this parameterization solution LazyParams that makes use of this feature, as demonstrated by test-examples at the project main page:
https://github.com/lazyparams/lazyparams
Last time I checked this was perfectly supported by Gradle, so I am puzzled to why this behaviour can suddenly be a problem.

When presenting LazyParams it has occurred to me that parameterization aside, the ability to conveniently add child nodes on all kinds of tests is in high demand !!
A recent example in this community is issue #5110, to which I added an example that only benefits from LazyParams' ability to add dynamic children, without introducing any more test-parameters.

Another example (to which I dont have any link) are desires to have assertions reported as dynamic child-nodes of their tests. I have been toying with a potential assertion frameworks, implemented a Jupiter extensions, that achieve this with the help of LazyParams. - Test result could look like this:
├─ JUnit Jupiter ✔
│ └─ RegularJupiterTest ✔
│ └─ regularTest() ✔
│ ├─ regularTest assert foo=23 ✔
│ ├─ regularTest assert bar contains 'non-alcoholic' ✔
│ └─ regularTest assert state='running' ✔

As of now these assertion features are not part of the LazyParams distribution, because they don't behave well when they are combined with other lazy parameters. But I intend to have them in future releases after improving the framework with abilities to add multiple layers of dynamic children.

So I'd find it unfortunate if Gradle community intend to introduce limits on the support for dynamicly added children. As I try to explain with the above examples, I believe the ability to easily add children dynamicly can unlock plenty of innovation around JUnit in the future and I hope for the Gradle community to continue to embrace this.