refactor: declare org.testng.internal null-marked

CHANGES.txt

@@ -9,11 +9,17 @@ Changed: GITHUB-3322: toXml() now declares the schema on <suite> instead of emit
 Changed: GITHUB-3322: Validating a suite against the schema requires a namespace-aware parser, which widens what counts as malformed: in a suite file that declares no doctype, an undeclared namespace prefix is now an error where it used to be read as part of the name. A suite declaring a doctype is unaffected, and testng.xml.validation=off restores the previous behaviour (Julien Herr)
 Changed: org.testng.internal.Utils.escapeHtml(String) and escapeUnicode(String) no longer accept null. Both used to answer null with null; they now throw a NullPointerException, and their return types are no longer nullable. No caller in TestNG passes null to either, and the null branch made the signature contradict itself once the package declares its nullness (Julien Herr)
 Changed: org.testng.reporters.XMLUtils.escape(String) no longer accepts null. It used to answer null with null; it now throws a NullPointerException, and its return type is no longer nullable. Nothing in TestNG ever called it with null, and nothing in TestNG calls it at all outside XMLUtils itself (Julien Herr)
+Changed: org.testng.internal.ClonedMethod.getConstructorOrMethod() returns the wrapped method instead of null. It answered null while the class held the java.lang.reflect.Method all along, so its own toString() threw a NullPointerException on every call, and none of the 59 call sites of ITestNGMethod.getConstructorOrMethod() in TestNG tested the result. Keeping it non-null is what lets ITestNGMethod.getConstructorOrMethod() stay non-null in the published interface (Julien Herr)
+Fixed: org.testng.internal.TestNGMethod.clone() no longer throws a NullPointerException when the method has not been bound to a test class yet. It wrapped getTestClass() in a NoOpTestClass, which dereferences it on the spot; the absence is now propagated, which is what ConfigurationMethod.clone() already did (Julien Herr)
+Fixed: A configuration method that is not a @BeforeGroups or @AfterGroups method now reports an empty array from getBeforeGroups() and getAfterGroups() instead of null. TestNGMethodFinder wrote null into fields whose declaration says {}, and MethodGroupsHelper iterates them without testing (Julien Herr)
+Fixed: Sorting test methods by index no longer throws a NullPointerException when a <test> tag carries no name (Julien Herr)
+Fixed: A <package> tag that carries no name attribute is now reported the way an unreadable package already was, instead of raising a NullPointerException from inside PackageUtils.findClassesInPackage (Julien Herr)
 
 Possible backward incompatible changes:
 
 - testng-failed.xml no longer records the index of a @Factory produced instance as an invocation-number. That attribute selects rows of a method's own data provider, which is the only thing TestNG ever reads it back as, so a factory powered failure produced a file that looked filtered and re-ran everything -- and, for a method that had a data provider of its own, re-ran the wrong rows because the factory index had overwritten the row index. The instance index now goes to the new factory-instances attribute of <include>, which is honoured on re-run. Tooling that parses testng-failed.xml to learn which factory instance failed must read factory-instances rather than invocation-numbers; a method with its own data provider now re-runs the rows that actually failed. A file generated by 7.13 and re-run by an older TestNG ignores the unknown attribute and re-runs every instance, which is what those versions already did. (GITHUB-3111, GITHUB-2517, GITHUB-2521)
 - The constructors of org.testng.internal.ParameterInfo and org.testng.internal.LazyParameterInfo now take an org.testng.internal.FactoryInstance instead of a loose index and parameter array. Both are implementation classes of an internal package; only code constructing them directly is affected. (GITHUB-3111)
+- org.testng.internal.ClonedMethod.getConstructorOrMethod() answers the wrapped method rather than null. Code that tested the result for null now takes the other branch; no caller in TestNG did, and the method's own toString() could never run before. ITestNGMethod.getConstructorOrMethod() therefore stays non-null when org.testng is marked in turn.
 - org.testng.internal.Utils.escapeHtml(String) and escapeUnicode(String) reject null instead of answering null with null. Both are public members of an internal, OSGi exported package: a Kotlin caller passing a String? stops compiling, and a Java caller passing null gets a NullPointerException from the first character read rather than a null result. Callers that relied on null-in/null-out must test for null themselves.
 - org.testng.reporters.XMLUtils.escape(String) rejects null instead of answering null with null. The package is now @NullMarked, so the parameter is declared non-null: a Kotlin caller passing a String? stops compiling, and a Java caller passing null gets a NullPointerException from the first character read rather than a null result. Callers that relied on null-in/null-out must test for null themselves.
 

testng-collections/src/main/java/org/testng/collections/MultiMap.java

@@ -22,7 +22,7 @@ protected MultiMap(boolean isSorted) {
 
   protected abstract C createValue();
 
-  public boolean put(K key, V method) {
+  public boolean put(@Nullable K key, V method) {
     AtomicBoolean exists = new AtomicBoolean(true);
     return m_objects
             .computeIfAbsent(
@@ -35,15 +35,15 @@ public boolean put(K key, V method) {
         && exists.get();
   }
 
-  public C get(K key) {
+  public C get(@Nullable K key) {
     return m_objects.computeIfAbsent(key, k -> createValue());
   }
 
   public Set<K> keySet() {
     return new HashSet<>(m_objects.keySet());
   }
 
-  public boolean containsKey(K k) {
+  public boolean containsKey(@Nullable K k) {
     return m_objects.containsKey(k);
   }
 
@@ -68,7 +68,7 @@ public int size() {
     return m_objects.size();
   }
 
-  public boolean remove(K key, V value) {
+  public boolean remove(@Nullable K key, V value) {
     return get(key).remove(value);
   }
 
@@ -78,7 +78,7 @@ public boolean remove(K key, V value) {
    * @param key the key to drop.
    * @return the values that were held, or {@code null} when the key was not present.
    */
-  public @Nullable C removeAll(K key) {
+  public @Nullable C removeAll(@Nullable K key) {
     return m_objects.remove(key);
   }
 
@@ -90,7 +90,7 @@ public Collection<C> values() {
     return m_objects.values();
   }
 
-  public boolean putAll(K k, Collection<? extends V> values) {
+  public boolean putAll(@Nullable K k, Collection<? extends V> values) {
     boolean result = false;
     for (V v : values) {
       result = put(k, v) || result;

testng-core-api/src/main/java/org/testng/internal/ConstructorOrMethod.java

@@ -49,6 +49,10 @@ public Class<?>[] getParameterTypes() {
     return member instanceof Method ? (Method) member : null;
   }
 
+  /**
+   * @return the wrapped member if it is a constructor, or {@code null} if it is a method. Prefer
+   *     {@link #requireConstructor()} unless the null is what you are testing for.
+   */
   public @Nullable Constructor<?> getConstructor() {
     return member instanceof Constructor ? (Constructor<?>) member : null;
   }
@@ -68,6 +72,21 @@ public Method requireMethod() {
     throw new NullPointerException("Expected a method, but " + member + " is a constructor");
   }
 
+  /**
+   * The wrapped member as a {@link Constructor}, for the callers that have already established it
+   * is not a method.
+   *
+   * @return the wrapped constructor
+   * @throws NullPointerException if this wrapper holds a method -- the same failure the call sites
+   *     saw before, with a message instead of a bare dereference
+   */
+  public Constructor<?> requireConstructor() {
+    if (member instanceof Constructor) {
+      return (Constructor<?>) member;
+    }
+    throw new NullPointerException("Expected a constructor, but " + member + " is a method");
+  }
+
   /**
    * Makes the wrapped member accessible. When interning is on the handle is shared, so this is
    * observable through every wrapper of the same member.

testng-core-api/src/main/java/org/testng/internal/Utils.java

@@ -286,7 +286,9 @@ public static void writeResourceToFile(File file, String resourceName, Class<?>
   }
 
   public static String defaultIfStringEmpty(@Nullable String s, String defaultValue) {
-    return isStringEmpty(s) ? defaultValue : s;
+    // Inlined rather than delegated to isStringEmpty: a nullness check hidden behind a call does
+    // not refine s on the other branch.
+    return s == null || s.isEmpty() ? defaultValue : s;
   }
 
   public static boolean isStringBlank(@Nullable String s) {

testng-core-api/src/main/java/org/testng/internal/package-info.java

@@ -0,0 +1,5 @@
+/** The engine behind the public API: test methods, parameters, configuration and scheduling. */
+@NullMarked
+package org.testng.internal;
+
+import org.jspecify.annotations.NullMarked;

testng-core-api/src/main/java/org/testng/xml/XmlPackage.java

@@ -65,8 +65,15 @@ public List<XmlClass> getXmlClasses() {
 
   private List<XmlClass> initializeXmlClasses() {
     List<XmlClass> result = new ArrayList<>();
+    String name = m_name;
+    if (name == null) {
+      // A <package> tag carrying no name attribute. Reported the same way as an unreadable
+      // package below, rather than through the NullPointerException this used to raise.
+      Utils.log("XmlPackage", 1, "Ignoring a <package> tag that carries no name.");
+      return result;
+    }
     try {
-      String[] classes = PackageUtils.findClassesInPackage(m_name, m_include, m_exclude);
+      String[] classes = PackageUtils.findClassesInPackage(name, m_include, m_exclude);
 
       int index = 0;
       for (String className : classes) {

testng-core-api/src/main/java/org/testng/xml/XmlWeaver.java

@@ -36,8 +36,7 @@ private static IWeaveXml instantiateIfRequired() {
       return instance;
     }
     Class<?> clazz = ClassHelper.forName(getClassName());
-    boolean isValid = clazz != null && IWeaveXml.class.isAssignableFrom(clazz);
-    if (!isValid) {
+    if (clazz == null || !IWeaveXml.class.isAssignableFrom(clazz)) {
       String msg =
           "In order for "
               + getClassName()

testng-core/src/main/java/org/testng/internal/BaseClassFinder.java

@@ -32,8 +32,8 @@ protected void putIClass(Class<?> cls, IClass iClass) {
   protected IClass findOrCreateIClass(
       ITestContext context,
       Class<?> cls,
-      XmlClass xmlClass,
-      IObject.IdentifiableObject instance,
+      @Nullable XmlClass xmlClass,
+      IObject.@Nullable IdentifiableObject instance,
       IAnnotationFinder annotationFinder,
       ITestObjectFactory objectFactory) {
 

testng-core/src/main/java/org/testng/internal/BaseTestMethod.java

@@ -90,7 +90,7 @@ public abstract class BaseTestMethod
   private int m_interceptedPriority;
 
   private @Nullable XmlTest m_xmlTest;
-  private final IObject.IdentifiableObject m_instance;
+  private final IObject.@Nullable IdentifiableObject m_instance;
 
   private final Map<String, IRetryAnalyzer> m_testMethodToRetryAnalyzer = new ConcurrentHashMap<>();
   protected final ITestObjectFactory m_objectFactory;
@@ -100,7 +100,7 @@ public BaseTestMethod(
       String methodName,
       ConstructorOrMethod com,
       IAnnotationFinder annotationFinder,
-      IObject.IdentifiableObject instance) {
+      IObject.@Nullable IdentifiableObject instance) {
     m_objectFactory = objectFactory;
     m_methodClass = com.getDeclaringClass();
     m_method = com;
@@ -133,7 +133,7 @@ public Class<?> getRealClass() {
 
   /** {@inheritDoc} */
   @Override
-  public void setTestClass(ITestClass tc) {
+  public void setTestClass(@Nullable ITestClass tc) {
     if (tc == null) {
       throw new IllegalArgumentException("test class cannot be null");
     }
@@ -175,7 +175,7 @@ public boolean isInstanceInstantiated() {
   }
 
   @Override
-  public UUID getInstanceId() {
+  public @Nullable UUID getInstanceId() {
     return Optional.ofNullable(m_instance)
         .map(IObject.IdentifiableObject::getInstanceId)
         .orElse(null);
@@ -187,6 +187,19 @@ public long[] getInstanceHashCodes() {
     return IObject.instanceHashCodes(m_testClass);
   }
 
+  /**
+   * The instance wrapper a clone of this method should carry: the same identity, or {@code null}
+   * when this method carries no instance at all.
+   */
+  protected IObject.@Nullable IdentifiableObject cloneInstance() {
+    Object instance = getInstance();
+    UUID instanceId = getInstanceId();
+    if (instance == null || instanceId == null) {
+      return null;
+    }
+    return new IObject.IdentifiableObject(instance, instanceId);
+  }
+
   /**
    * {@inheritDoc}
    *
@@ -465,15 +478,15 @@ protected void initGroups(Class<? extends ITestOrConfiguration> annotationClass)
 
   protected void initBeforeAfterGroups(
       Class<? extends ITestOrConfiguration> annotationClass, String[] groups) {
-    String[] groupsAtMethodLevel =
+    String @Nullable [] groupsAtMethodLevel =
         calculateGroupsToUseConsideringValuesAndGroupValues(annotationClass, groups);
     // @BeforeGroups and @AfterGroups annotation cannot be used at Class level. So its always null
     setGroups(getStringArray(groupsAtMethodLevel, null));
     initRestOfGroupDependencies(annotationClass);
   }
 
-  private String[] calculateGroupsToUseConsideringValuesAndGroupValues(
-      Class<? extends ITestOrConfiguration> annotationClass, String[] groups) {
+  private String @Nullable [] calculateGroupsToUseConsideringValuesAndGroupValues(
+      Class<? extends ITestOrConfiguration> annotationClass, String @Nullable [] groups) {
     if (groups == null || groups.length == 0) {
       ITestOrConfiguration annotation =
           getAnnotationFinder().findAnnotation(getConstructorOrMethod(), annotationClass);
@@ -521,7 +534,7 @@ private void initRestOfGroupDependencies(Class<? extends ITestOrConfiguration> a
     setMethodsDependedUpon(methodsDependedUpon);
   }
 
-  private static Map<String, Set<String>> calculateXmlGroupDependencies(XmlTest xmlTest) {
+  private static Map<String, Set<String>> calculateXmlGroupDependencies(@Nullable XmlTest xmlTest) {
     Map<String, Set<String>> result = new HashMap<>();
     if (xmlTest == null) {
       return result;
@@ -654,7 +667,7 @@ public void addMethodDependedUpon(String method) {
 
   /** {@inheritDoc} */
   @Override
-  public void setMissingGroup(String group) {
+  public void setMissingGroup(@Nullable String group) {
     m_missingGroup = group;
   }
 
@@ -669,7 +682,7 @@ public int getThreadPoolSize() {
   public void setThreadPoolSize(int threadPoolSize) {}
 
   @Override
-  public void setDescription(String description) {
+  public void setDescription(@Nullable String description) {
     m_description = description;
   }
 
@@ -847,7 +860,7 @@ public void setInterceptedPriority(int priority) {
     return m_xmlTest;
   }
 
-  public void setXmlTest(XmlTest xmlTest) {
+  public void setXmlTest(@Nullable XmlTest xmlTest) {
     m_xmlTest = xmlTest;
   }
 
@@ -863,7 +876,11 @@ public Class<?>[] getParameterTypes() {
 
   @Override
   public Map<String, String> findMethodParameters(XmlTest test) {
-    return XmlTestUtils.findMethodParameters(test, getTestClass().getName(), getMethodName());
+    // No test class bound yet means no <class> tag can match, which XmlTestUtils answers with
+    // the suite and <test> parameters on their own.
+    ITestClass testClass = getTestClass();
+    return XmlTestUtils.findMethodParameters(
+        test, testClass == null ? null : testClass.getName(), getMethodName());
   }
 
   @Override
@@ -873,7 +890,7 @@ public String getQualifiedName() {
 
   @Override
   @Deprecated
-  public IParameterInfo getFactoryMethodParamsInfo() {
+  public @Nullable IParameterInfo getFactoryMethodParamsInfo() {
     return getFactoryParameterInfo();
   }
 
@@ -916,7 +933,7 @@ private static boolean isNotParameterisedTest(ITestResult tr) {
     return Optional.ofNullable(tr.getParameters()).orElse(new Object[0]).length == 0;
   }
 
-  private IRetryAnalyzer computeRetryAnalyzerInstanceToUse(ITestResult tr) {
+  private @Nullable IRetryAnalyzer computeRetryAnalyzerInstanceToUse(ITestResult tr) {
     if (m_retryAnalyzer != null) {
       return m_retryAnalyzer;
     }

testng-core/src/main/java/org/testng/internal/ClassImpl.java

@@ -30,17 +30,17 @@ public class ClassImpl implements IClass, IObject {
   private final List<IObject.IdentifiableObject> identifiableObjects = new ArrayList<>();
   private final Map<Class<?>, IClass> m_classes;
   private long @Nullable [] m_instanceHashCodes;
-  private final IObject.IdentifiableObject m_instance;
+  private final IObject.@Nullable IdentifiableObject m_instance;
   private final ITestObjectFactory m_objectFactory;
   private @Nullable String m_testName = null;
-  private final XmlClass m_xmlClass;
+  private final @Nullable XmlClass m_xmlClass;
   private final ITestContext m_testContext;
 
   public ClassImpl(
       ITestContext context,
       Class<?> cls,
-      XmlClass xmlClass,
-      IObject.IdentifiableObject instance,
+      @Nullable XmlClass xmlClass,
+      IObject.@Nullable IdentifiableObject instance,
       Map<Class<?>, IClass> classes,
       IAnnotationFinder annotationFinder,
       ITestObjectFactory objectFactory) {
@@ -51,8 +51,9 @@ public ClassImpl(
     m_annotationFinder = annotationFinder;
     m_instance = instance;
     m_objectFactory = objectFactory;
-    if (IObject.IdentifiableObject.unwrap(instance) instanceof ITest) {
-      m_testName = ((ITest) instance.getInstance()).getTestName();
+    Object unwrapped = IObject.IdentifiableObject.unwrap(instance);
+    if (unwrapped instanceof ITest) {
+      m_testName = ((ITest) unwrapped).getTestName();
     }
     if (m_testName == null) {
       ITestAnnotation annotation = m_annotationFinder.findAnnotation(cls, ITestAnnotation.class);
@@ -88,11 +89,12 @@ public XmlTest getXmlTest() {
   }
 
   @Override
-  public XmlClass getXmlClass() {
+  public @Nullable XmlClass getXmlClass() {
     return m_xmlClass;
   }
 
-  private IObject.IdentifiableObject getDefaultInstance(boolean create, String errMsgPrefix) {
+  private IObject.@Nullable IdentifiableObject getDefaultInstance(
+      boolean create, String errMsgPrefix) {
     if (m_defaultInstance == null) {
       if (m_instance != null) {
         m_defaultInstance = m_instance;
@@ -170,7 +172,9 @@ private static int computeHashCode(IdentifiableObject identifiable) {
       // derive a stable one from its unique instance id instead.
       return identifiable.getInstanceId().hashCode();
     }
-    return IParameterInfo.embeddedInstance(instance).hashCode();
+    return java.util.Objects.requireNonNull(
+            IParameterInfo.embeddedInstance(instance), "the factory instance is not available")
+        .hashCode();
   }
 
   private DetailedAttributes newDetailedAttributes(boolean create, String errMsgPrefix) {