Merge pull request #1630 from krmahadevan/krmahadevan-fix-1625

Null fields in parallel method tests

Author: krmahadevan

CHANGES.txt

@@ -1,9 +1,11 @@
 Current
 =======
+Fixed: GITHUB-1625: Null fields in parallel method tests (Krishnan Mahadevan)
 
-6.13
-====
+6.13.1
+No functional changes. Released with newer version JCommander (1.72.0)
 
+6.13
 Fixed: GITHUB-1619: ConcurrentHashMap doesn't secure insertion order.(Yehui Wang)
 Fixed: GITHUB-1616: Test cases with priority and dependsOnGroups dependencies, execution order is chaos. (Yehui Wang)
 Fixed: GITHUB-1613: The constructor removed from TestRunner would stop Eclipse working.(Yehui Wang)

build.gradle

@@ -89,6 +89,7 @@ dependencies {
     testCompile 'org.assertj:assertj-core:2.5.0'
     testCompile 'org.codehaus.groovy:groovy-all:2.4.7'
     testCompile 'org.spockframework:spock-core:1.0-groovy-2.4'
+    testCompile 'org.mockito:mockito-core:2.12.0'
 }
 
 task sourceJar(type: Jar) {

kobalt/src/Build.kt

@@ -64,6 +64,7 @@ val p = project {
     dependenciesTest {
         compile("org.assertj:assertj-core:3.5.2",
                 "org.testng:testng:6.9.13.7",
+                "org.mockito:mockito-core:2.12.0",
                 "org.spockframework:spock-core:1.0-groovy-2.4")
     }
 

src/main/java/org/testng/internal/TestMethodWorker.java

@@ -148,24 +148,30 @@ protected void invokeBeforeClassMethods(ITestClass testClass, IMethodInstance mi
 
     // the whole invocation must be synchronized as other threads must
     // get a full initialized test object (not the same for @After)
-    Map<ITestClass, Set<Object>> invokedBeforeClassMethods = m_classMethodMap.getInvokedBeforeClassMethods();
-    Set<Object> instances = invokedBeforeClassMethods.get(testClass);
-    if (null == instances) {
-      instances = new HashSet<>();
-      invokedBeforeClassMethods.put(testClass, instances);
-    }
-    Object instance = mi.getInstance();
-    if (!instances.contains(instance)) {
-      instances.add(instance);
-      for (IClassListener listener : m_listeners) {
-        listener.onBeforeClass(testClass);
+    // Synchronization on local variables is generally considered a bad practice, but this is an exception.
+    // We need to ensure that two threads that are querying for the same "Class" then they
+    // should be mutually exclusive. In all other cases, parallelism can be allowed.
+    //DO NOT REMOVE THIS SYNC LOCK.
+    synchronized (testClass) {
+      Map<ITestClass, Set<Object>> invokedBeforeClassMethods = m_classMethodMap.getInvokedBeforeClassMethods();
+      Set<Object> instances = invokedBeforeClassMethods.get(testClass);
+      if (null == instances) {
+        instances = new HashSet<>();
+        invokedBeforeClassMethods.put(testClass, instances);
+      }
+      Object instance = mi.getInstance();
+      if (!instances.contains(instance)) {
+        instances.add(instance);
+        for (IClassListener listener : m_listeners) {
+          listener.onBeforeClass(testClass);
+        }
+        m_invoker.invokeConfigurations(testClass,
+                testClass.getBeforeClassMethods(),
+                m_suite,
+                m_parameters,
+                null, /* no parameter values */
+                instance);
       }
-      m_invoker.invokeConfigurations(testClass,
-              testClass.getBeforeClassMethods(),
-              m_suite,
-              m_parameters,
-              null, /* no parameter values */
-              instance);
     }
   }
 

src/test/java/test/configuration/BeforeClassThreadTest.java

@@ -5,16 +5,16 @@
 import org.testng.xml.XmlSuite;
 
 import test.SimpleBaseTest;
-import junit.framework.Assert;
 
-public class BeforeClassThreadTest extends SimpleBaseTest{
+import static org.assertj.core.api.Assertions.assertThat;
+
+public class BeforeClassThreadTest extends SimpleBaseTest {
 
     @Test
     public void beforeClassMethodsShouldRunInParallel() {
-        TestNG tng = create(new Class[] { BeforeClassThreadA.class, BeforeClassThreadB.class });
+        TestNG tng = create(BeforeClassThreadA.class, BeforeClassThreadB.class);
         tng.setParallel(XmlSuite.ParallelMode.METHODS);
         tng.run();
-
-        Assert.assertTrue(Math.abs(BeforeClassThreadA.WHEN - BeforeClassThreadB.WHEN) < 1000);
+        assertThat(Math.abs(BeforeClassThreadA.WHEN - BeforeClassThreadB.WHEN)).isLessThan(1000);
     }
 }

src/test/java/test/configuration/github1625/TestRunnerIssue1625.java

@@ -0,0 +1,28 @@
+package test.configuration.github1625;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+import org.testng.TestNG;
+import org.testng.annotations.DataProvider;
+import org.testng.annotations.Test;
+import org.testng.xml.XmlSuite;
+import test.SimpleBaseTest;
+
+public class TestRunnerIssue1625 extends SimpleBaseTest {
+
+    @Test(dataProvider = "dp")
+    public void testMethod(Class<?> clazz) {
+        TestNG testNG = create(clazz);
+        testNG.setParallel(XmlSuite.ParallelMode.METHODS);
+        testNG.run();
+        assertThat(testNG.getStatus()).isEqualTo(0);
+    }
+
+    @DataProvider(name = "dp")
+    public Object[][] getData() {
+        return new Object[][]{
+                {TestclassSampleUsingMocks.class},
+                {TestclassSampleWithoutUsingMocks.class}
+        };
+    }
+}

src/test/java/test/configuration/github1625/TestclassSampleUsingMocks.java

@@ -0,0 +1,30 @@
+package test.configuration.github1625;
+
+import org.mockito.Mock;
+import org.mockito.MockitoAnnotations;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import java.util.List;
+
+public class TestclassSampleUsingMocks {
+    @Mock
+    List<String> list;
+
+
+    @BeforeClass
+    public void beforeClass() {
+        MockitoAnnotations.initMocks(this);
+    }
+
+    @Test
+    public void first() {
+        Assert.assertNotNull(list);
+    }
+
+    @Test
+    public void second() {
+        Assert.assertNotNull(list);
+    }
+}

src/test/java/test/configuration/github1625/TestclassSampleWithoutUsingMocks.java

@@ -0,0 +1,28 @@
+package test.configuration.github1625;
+
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+import java.util.ArrayList;
+import java.util.List;
+
+public class TestclassSampleWithoutUsingMocks {
+
+    private List<String> list;
+
+    @BeforeClass
+    public void beforeClass() {
+        list = new ArrayList<>();
+    }
+
+    @Test
+    public void first() {
+        Assert.assertNotNull(list);
+    }
+
+    @Test
+    public void second() {
+        Assert.assertNotNull(list);
+    }
+}

src/test/resources/testng.xml

@@ -782,6 +782,7 @@
       <class name="test.configuration.VerifySuiteTest" />
       <class name="test.configuration.SingleConfigurationTest" />
       <class name="test.configuration.BeforeClassWithDisabledTest" />
+      <class name="test.configuration.github1625.TestRunnerIssue1625"/>
     </classes>
   </test>