Skip to content

Commit

Permalink
Merge pull request #2753 from velma/fix-aftergroups
Browse files Browse the repository at this point in the history 
fix possibilty that AfterGroups method is invoked before all tests
  • Loading branch information
@juherr
juherr committed Apr 27, 2022
2 parents aae0724 + 6fd891f commit 28d0fd4
Show file tree
Hide file tree
Showing 5 changed files with 139 additions and 100 deletions.
1 change: 1 addition & 0 deletions CHANGES.txt
View file
Expand Up @@ -11,6 +11,7 @@ Fixed: GITHUB-2637: Upgrade to JDK11 as the minimum JDK requirements (Krishnan M
Fixed: GITHUB-2734: Keep the initial order of listeners (Andrei Solntsev)
Fixed: GITHUB-2359: Testng @BeforeGroups is running in parallel with testcases in the group (Anton Velma)
Fixed: Possible StringIndexOutOfBoundsException in XmlReporter (Anton Velma)
Fixed: GITHUB-2754: @AfterGroups is executed for each "finished" group when it has multiple groups defined (Anton Velma)

7.5
Fixed: GITHUB-2701: Bump gradle version to 7.3.3 to support java17 build (ZhangJian He)
Expand Down
122 changes: 70 additions & 52 deletions testng-core/src/main/java/org/testng/internal/ConfigurationGroupMethods.java
View file
Expand Up @@ -3,6 +3,7 @@
import java.util.Arrays;
import java.util.Collection;
import java.util.Collections;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Objects;
Expand All @@ -11,6 +12,7 @@
import java.util.concurrent.CountDownLatch;
import java.util.stream.Collectors;
import org.testng.ITestNGMethod;
import org.testng.collections.CollectionUtils;
import org.testng.collections.Lists;
import org.testng.collections.Maps;
import org.testng.log4testng.Logger;
Expand Down Expand Up @@ -57,56 +59,6 @@ public Map<String, List<ITestNGMethod>> getAfterGroupsMethods() {
return m_afterGroupsMethods;
}

/**
* @param group The group name
* @param method The test method
* @return true if the passed method is the last to run for the group. This method is used to
* figure out when is the right time to invoke afterGroups methods.
*/
public boolean isLastMethodForGroup(String group, ITestNGMethod method) {

// If we have more invocation to do, this is not the last one yet
if (method.hasMoreInvocation()) {
return false;
}

// This Mutex ensures that this edit check runs sequentially for one ITestNGMethod
// method at a time because this object is being shared between all the ITestNGMethod objects.
synchronized (this) {
if (m_afterGroupsMap == null) {
m_afterGroupsMap = initializeAfterGroupsMap();
}

List<ITestNGMethod> methodsInGroup = m_afterGroupsMap.get(group);

if (null == methodsInGroup || methodsInGroup.isEmpty()) {
return false;
}

methodsInGroup.remove(method);

// Note: == is not good enough here as we may work with ITestNGMethod clones
return methodsInGroup.isEmpty();
}
}

private Map<String, List<ITestNGMethod>> initializeAfterGroupsMap() {
Map<String, List<ITestNGMethod>> result = Maps.newConcurrentMap();
for (ITestNGMethod m : m_allMethods) {
String[] groups = m.getGroups();
for (String g : groups) {
List<ITestNGMethod> methodsInGroup = result.computeIfAbsent(g, key -> Lists.newArrayList());
methodsInGroup.add(m);
}
}

synchronized (afterGroupsThatHaveAlreadyRun) {
afterGroupsThatHaveAlreadyRun.clear();
}

return result;
}

public List<ITestNGMethod> getBeforeGroupMethodsForGroup(String[] groups) {
if (groups.length == 0) {
return Collections.emptyList();
Expand All @@ -121,10 +73,40 @@ public List<ITestNGMethod> getBeforeGroupMethodsForGroup(String[] groups) {
}
}

public List<ITestNGMethod> getAfterGroupMethodsForGroup(String group) {
public List<ITestNGMethod> getAfterGroupMethods(ITestNGMethod testMethod) {
if (testMethod.hasMoreInvocation() || testMethod.getGroups().length == 0) {
return Collections.emptyList();
}

Set<String> methodGroups = new HashSet<>(Arrays.asList(testMethod.getGroups()));

synchronized (afterGroupsThatHaveAlreadyRun) {
return retrieve(afterGroupsThatHaveAlreadyRun, m_afterGroupsMethods, group);
if (m_afterGroupsMap == null) {
m_afterGroupsMap = initializeAfterGroupsMap();
}

return methodGroups.stream()
.filter(t -> isLastMethodForGroup(t, testMethod))
.map(t -> retrieve(afterGroupsThatHaveAlreadyRun, m_afterGroupsMethods, t))
.filter(Objects::nonNull)
.flatMap(Collection::stream)
.filter(t -> isAfterGroupAllowedToRunAfterTestMethod(t, methodGroups))
.collect(Collectors.toList());
}
}

private boolean isAfterGroupAllowedToRunAfterTestMethod(
ITestNGMethod afterGroupMethod, Set<String> testMethodGroups) {
String[] afterGroupMethodGroups = afterGroupMethod.getAfterGroups();
if (afterGroupMethodGroups.length == 1
|| testMethodGroups.containsAll(Arrays.asList(afterGroupMethodGroups))) {
return true;
}
return Arrays.stream(afterGroupMethodGroups)
.allMatch(
t ->
testMethodGroups.contains(t)
|| !CollectionUtils.hasElements(m_afterGroupsMap.get(t)));
}

public void removeBeforeGroups(String[] groups) {
Expand All @@ -140,6 +122,42 @@ public void removeAfterGroups(Collection<String> groups) {
}
}

/**
* @param group The group name
* @param method The test method
* @return true if the passed method is the last to run for the group. This method is used to
* figure out when is the right time to invoke afterGroups methods.
*/
private boolean isLastMethodForGroup(String group, ITestNGMethod method) {
List<ITestNGMethod> methodsInGroup = m_afterGroupsMap.get(group);

if (null == methodsInGroup || methodsInGroup.isEmpty()) {
return true;
}

methodsInGroup.remove(method);

// Note: == is not good enough here as we may work with ITestNGMethod clones
return methodsInGroup.isEmpty();
}

private Map<String, List<ITestNGMethod>> initializeAfterGroupsMap() {
Map<String, List<ITestNGMethod>> result = Maps.newConcurrentMap();
for (ITestNGMethod m : m_allMethods) {
String[] groups = m.getGroups();
for (String g : groups) {
List<ITestNGMethod> methodsInGroup = result.computeIfAbsent(g, key -> Lists.newArrayList());
methodsInGroup.add(m);
}
}

synchronized (afterGroupsThatHaveAlreadyRun) {
afterGroupsThatHaveAlreadyRun.clear();
}

return result;
}

private static List<ITestNGMethod> retrieve(
Map<String, CountDownLatch> tracker, Map<String, List<ITestNGMethod>> map, String group) {
if (tracker.containsKey(group)) {
Expand Down
77 changes: 29 additions & 48 deletions testng-core/src/main/java/org/testng/internal/invokers/ConfigInvoker.java
View file
Expand Up @@ -7,7 +7,7 @@
import java.lang.reflect.InvocationTargetException;
import java.util.Arrays;
import java.util.Collection;
import java.util.List;
import java.util.HashSet;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
Expand All @@ -24,7 +24,17 @@
import org.testng.annotations.IConfigurationAnnotation;
import org.testng.collections.Maps;
import org.testng.collections.Sets;
import org.testng.internal.*;
import org.testng.internal.ClassHelper;
import org.testng.internal.ConfigurationMethod;
import org.testng.internal.ConstructorOrMethod;
import org.testng.internal.IConfiguration;
import org.testng.internal.ITestResultNotifier;
import org.testng.internal.MethodHelper;
import org.testng.internal.Parameters;
import org.testng.internal.RuntimeBehavior;
import org.testng.internal.TestListenerHelper;
import org.testng.internal.TestResult;
import org.testng.internal.Utils;
import org.testng.internal.annotations.AnnotationHelper;
import org.testng.internal.invokers.ConfigMethodArguments.Builder;
import org.testng.internal.thread.ThreadUtil;
Expand Down Expand Up @@ -159,59 +169,30 @@ public void invokeAfterGroupsConfigurations(GroupConfigMethodArguments arguments
return;
}

// See if the currentMethod is the last method in any of the groups
// it belongs to
Map<String, String> filteredGroups = Maps.newHashMap();
String[] groups = arguments.getTestMethod().getGroups();
for (String group : groups) {
if (arguments.getGroupMethods().isLastMethodForGroup(group, arguments.getTestMethod())) {
filteredGroups.put(group, group);
}
}

if (filteredGroups.isEmpty()) {
return;
}

// The list of afterMethods to run
Map<ITestNGMethod, ITestNGMethod> afterMethods = Maps.newHashMap();

// Now filteredGroups contains all the groups for which we need to run the afterGroups
// method. Find all the methods that correspond to these groups and invoke them.
for (String g : filteredGroups.values()) {
List<ITestNGMethod> methods = arguments.getGroupMethods().getAfterGroupMethodsForGroup(g);
// Note: should put them in a map if we want to make sure the same afterGroups
// doesn't get run twice
if (methods != null) {
for (ITestNGMethod m : methods) {
afterMethods.put(m, m);
}
}
}

// Got our afterMethods, invoke them
Set<String> filteredGroups = new HashSet<>();
ITestNGMethod[] filteredConfigurations =
afterMethods.keySet().stream()
arguments.getGroupMethods().getAfterGroupMethods(arguments.getTestMethod()).stream()
.peek(t -> filteredGroups.addAll(Arrays.asList(t.getGroups())))
.filter(ConfigInvoker::isGroupLevelConfigurationMethod)
.toArray(ITestNGMethod[]::new);
if (filteredConfigurations.length == 0) {
return;
if (filteredConfigurations.length != 0) {
// don't pass the IClass or the instance as the method may be external
// the invocation must be similar to @BeforeTest/@BeforeSuite
ConfigMethodArguments configMethodArguments =
new Builder()
.usingConfigMethodsAs(filteredConfigurations)
.forSuite(arguments.getSuite())
.usingParameters(arguments.getParameters())
.usingInstance(arguments.getInstance())
.forTestMethod(arguments.getTestMethod())
.build();

invokeConfigurations(configMethodArguments);
}
// don't pass the IClass or the instance as the method may be external
// the invocation must be similar to @BeforeTest/@BeforeSuite
ConfigMethodArguments configMethodArguments =
new Builder()
.usingConfigMethodsAs(filteredConfigurations)
.forSuite(arguments.getSuite())
.usingParameters(arguments.getParameters())
.usingInstance(arguments.getInstance())
.forTestMethod(arguments.getTestMethod())
.build();

invokeConfigurations(configMethodArguments);

// Remove the groups so they don't get run again
arguments.getGroupMethods().removeAfterGroups(filteredGroups.keySet());
arguments.getGroupMethods().removeAfterGroups(filteredGroups);
}

public void invokeConfigurations(ConfigMethodArguments arguments) {
Expand Down
21 changes: 21 additions & 0 deletions testng-core/src/test/java/test/aftergroups/AfterGroupsBehaviorTest.java
View file
Expand Up @@ -2,6 +2,7 @@

import static org.assertj.core.api.Assertions.assertThat;

import org.testng.ITestResult;
import org.testng.TestNG;
import org.testng.annotations.DataProvider;
import org.testng.annotations.Test;
Expand All @@ -12,6 +13,8 @@
import test.aftergroups.issue165.TestclassSampleWithSkippedMember;
import test.aftergroups.issue1880.LocalConfigListener;
import test.aftergroups.issue1880.TestClassSample;
import test.aftergroups.samples.MultipleGroupsSample;
import test.beforegroups.issue2359.ListenerAdapter;

public class AfterGroupsBehaviorTest extends SimpleBaseTest {

Expand All @@ -33,6 +36,24 @@ public Object[][] getData() {
};
}

@Test
public void ensureAfterGroupsInvokedAfterAllTestsWhenMultipleGroupsDefined() {
TestNG tng = new TestNG();
tng.setTestClasses(new Class[] {MultipleGroupsSample.class});

ListenerAdapter adapter = new ListenerAdapter();
tng.addListener(adapter);

tng.run();

assertThat(adapter.getPassedConfiguration()).hasSize(1);
ITestResult afterGroup = adapter.getPassedConfiguration().iterator().next();
adapter
.getPassedTests()
.forEach(
t -> assertThat(t.getEndMillis()).isLessThanOrEqualTo(afterGroup.getStartMillis()));
}

private static void runTest(
Class<?> clazz, String groups, boolean shouldContinue, String expected) {
XmlSuite xmlsuite = createXmlSuite("sample_suite", "sample_test", clazz);
Expand Down
18 changes: 18 additions & 0 deletions testng-core/src/test/java/test/aftergroups/samples/MultipleGroupsSample.java
View file
@@ -0,0 +1,18 @@
package test.aftergroups.samples;

import org.testng.annotations.AfterGroups;
import org.testng.annotations.Test;

public class MultipleGroupsSample {

@AfterGroups(groups = {"group-1", "group-2", "not-defined"})
public void afterGroup() {}

@Test(groups = "group-1")
public void test1() {}

@Test(groups = "group-2")
public void test2() throws InterruptedException {
Thread.sleep(3000);
}
}

0 comments on commit 28d0fd4

Please sign in to comment.