Clarify test descriptor implementation requirements (#5170)

* Define test and container as non-exclusive properties.
* Use emphasis to distinguish between term and word.
* Document that `TestDescriptor.Type` values are exclusive properties.
* Document that `isContainer()` and `isTest()` must be consistent with `getType()`.
* Document that `getChildren()`, `getDescendants()` and `mayRegisterTests()` must be consistent with `isContainer()`.
* Document that only the hierarchy of the test descriptor is mutable after discovery.

Author: mpkorstanje

documentation/src/docs/asciidoc/release-notes/release-notes-6.1.0-M2.adoc

@@ -16,7 +16,7 @@ repository on GitHub.
 [[release-notes-6.1.0-M2-junit-platform-bug-fixes]]
 ==== Bug Fixes
 
-* ❓
+* Clarify `TestDescriptor` implementation requirements.
 
 [[release-notes-6.1.0-M2-junit-platform-deprecations-and-breaking-changes]]
 ==== Deprecations and Breaking Changes

documentation/src/docs/asciidoc/user-guide/advanced-topics/engines.adoc

@@ -95,6 +95,8 @@ to the following requirements:
 * The `TestDescriptor` returned from `TestEngine.discover()` _must_ be the root of a tree
   of `TestDescriptor` instances. This implies that there _must not_ be any cycles between
   a node and its descendants.
+* The hierarchy of test descriptors returned from `TestEngine.discover()` _must_ be
+  mutable, but the test descriptors _must_ otherwise be immutable after discovery.
 * A `TestEngine` _must_ be able to discover `UniqueIdSelectors` for any unique ID that it
   previously generated and returned from `TestEngine.discover()`. This enables selecting a
   subset of tests to execute or rerun.

junit-platform-engine/src/main/java/org/junit/platform/engine/TestDescriptor.java

@@ -27,8 +27,8 @@
 import org.junit.platform.commons.util.Preconditions;
 
 /**
- * Mutable descriptor for a test or container that has been discovered by a
- * {@link TestEngine}.
+ * A descriptor with a mutable hierarchy for a test or container that has been
+ * discovered by a {@link TestEngine}.
  *
  * @since 1.0
  * @see TestEngine
@@ -42,6 +42,9 @@ public interface TestDescriptor {
 	 * <p>Uniqueness must be guaranteed across an entire test plan,
 	 * regardless of how many engines are used behind the scenes.
 	 *
+	 * <p>The implementation must treat this property as immutable after test
+	 * discovery has completed.
+	 *
 	 * @return the {@code UniqueId} for this descriptor; never {@code null}
 	 */
 	UniqueId getUniqueId();
@@ -77,6 +80,9 @@ default String getLegacyReportingName() {
 	/**
 	 * Get the set of {@linkplain TestTag tags} associated with this descriptor.
 	 *
+	 * <p>The implementation must treat this property as immutable after test
+	 * discovery has completed.
+	 *
 	 * @return the set of tags associated with this descriptor; never {@code null}
 	 * but potentially empty
 	 * @see TestTag
@@ -87,6 +93,9 @@ default String getLegacyReportingName() {
 	 * Get the {@linkplain TestSource source} of the test or container described
 	 * by this descriptor, if available.
 	 *
+	 * <p>The implementation must treat this property as immutable after test
+	 * discovery has completed.
+	 *
 	 * @see TestSource
 	 */
 	Optional<TestSource> getSource();
@@ -106,6 +115,9 @@ default String getLegacyReportingName() {
 	/**
 	 * Get the immutable set of <em>children</em> of this descriptor.
 	 *
+	 * <p>The implementation must be consistent with {@link #isContainer()} such that
+	 * {@code !x.container()} implies {@code x.getChildren().isEmpty()}.
+	 *
 	 * @return the set of children of this descriptor; neither {@code null}
 	 * nor mutable, but potentially empty
 	 * @see #getDescendants()
@@ -141,6 +153,9 @@ default Set<? extends TestDescriptor> getAncestors() {
 	 * <p>A <em>descendant</em> is a child of this descriptor or a child of one of
 	 * its children, recursively.
 	 *
+	 * <p>The implementation must be consistent with {@link #isContainer()} such that
+	 * {@code !x.container()} implies {@code x.getDescendants().isEmpty()}.
+	 *
 	 * @see #getChildren()
 	 */
 	default Set<? extends TestDescriptor> getDescendants() {
@@ -223,14 +238,24 @@ default boolean isRoot() {
 	/**
 	 * Determine the {@link Type} of this descriptor.
 	 *
+	 * <p>The implementation must treat this property as immutable after test
+	 * discovery has completed.
+	 *
 	 * @return the descriptor type; never {@code null}.
 	 * @see #isContainer()
 	 * @see #isTest()
 	 */
 	Type getType();
 
 	/**
-	 * Determine if this descriptor describes a container.
+	 * Determine if this descriptor describes a <em>container</em>.
+	 *
+	 * <p>A test descriptor is a <em>container</em> when it may contain other
+	 * containers or tests as its children. In addition to being a
+	 * <em>container</em> this test descriptor may also be a <em>test</em>.
+	 *
+	 * <p>The implementation must be consistent with {@link #getType()} such
+	 * that {@code x.isContainer()} equals {@code x.getType().isContainer()}.
 	 *
 	 * <p>The default implementation delegates to {@link Type#isContainer()}.
 	 */
@@ -239,7 +264,14 @@ default boolean isContainer() {
 	}
 
 	/**
-	 * Determine if this descriptor describes a test.
+	 * Determine if this descriptor describes a <em>test</em>.
+	 *
+	 * <p>A test descriptor is a <em>test</em> when it verifies expected
+	 * behavior when executed. In addition to being a <em>test</em> this
+	 * test descriptor may also be a <em>container</em>.
+	 *
+	 * <p>The implementation must be consistent with {@link #getType()} such
+	 * that {@code x.isTest()} equals {@code x.getType().isTest()}.
 	 *
 	 * <p>The default implementation delegates to {@link Type#isTest()}.
 	 */
@@ -250,6 +282,10 @@ default boolean isTest() {
 	/**
 	 * Determine if this descriptor may register dynamic tests during execution.
 	 *
+	 * <p>The implementation must treat this property as immutable after test
+	 * discovery has completed and must be consistent with {@link #isContainer()}
+	 * such that {@code !x.container()} implies {@code !x.mayRegisterTests()}.
+	 *
 	 * <p>The default implementation assumes tests are usually known during
 	 * discovery and thus returns {@code false}.
 	 */
@@ -259,7 +295,7 @@ default boolean mayRegisterTests() {
 
 	/**
 	 * Determine if the supplied descriptor (or any of its descendants)
-	 * {@linkplain TestDescriptor#isTest() is a test} or
+	 * {@linkplain TestDescriptor#isTest() is a <em>test</em>} or
 	 * {@linkplain TestDescriptor#mayRegisterTests() may potentially register
 	 * tests dynamically}.
 	 *
@@ -355,12 +391,14 @@ static Visitor composite(Visitor... visitors) {
 	enum Type {
 
 		/**
-		 * Denotes that the {@link TestDescriptor} is for a <em>container</em>.
+		 * Denotes that the {@link TestDescriptor} is strictly for a
+		 * <em>container</em>. I.e. it is not also a <em>test</em>.
 		 */
 		CONTAINER,
 
 		/**
-		 * Denotes that the {@link TestDescriptor} is for a <em>test</em>.
+		 * Denotes that the {@link TestDescriptor} is strictly for a
+		 * <em>test</em>. I.e. it is not also a <em>container</em>.
 		 */
 		TEST,