Skip to content

Commit

Permalink
[SUREFIRE-1264] Some tests can be lost when running in parallel with …
Browse files Browse the repository at this point in the history
…parameterized tests
  • Loading branch information
Tibor17 committed May 4, 2017
1 parent 4e993a0 commit 92b39d6
Show file tree
Hide file tree
Showing 6 changed files with 219 additions and 31 deletions.
@@ -1 +1,52 @@
package org.apache.maven.surefire.util.internal;/* * Licensed to the Apache Software Foundation (ASF) under one * or more contributor license agreements. See the NOTICE file * distributed with this work for additional information * regarding copyright ownership. The ASF licenses this file * to you under the Apache License, Version 2.0 (the * "License"); you may not use this file except in compliance * with the License. You may obtain a copy of the License at * * http://www.apache.org/licenses/LICENSE-2.0 * * Unless required by applicable law or agreed to in writing, * software distributed under the License is distributed on an * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY * KIND, either express or implied. See the License for the * specific language governing permissions and limitations * under the License. */import java.util.regex.Matcher;import java.util.regex.Pattern;/** * JUnit Description parser. * Used by JUnit Version lower than 4.7. * * @author <a href="mailto:tibordigana@apache.org">Tibor Digana (tibor17)</a> * @since 2.20 */public final class TestClassMethodNameUtils{ private static final Pattern METHOD_CLASS_PATTERN = Pattern.compile( "([\\s\\S]*)\\((.*)\\)" ); private TestClassMethodNameUtils() { throw new IllegalStateException( "no instantiable constructor" ); } public static String extractClassName( String displayName ) { Matcher m = METHOD_CLASS_PATTERN.matcher( displayName ); return m.matches() ? m.group( 2 ) : displayName; } public static String extractMethodName( String displayName ) { int i = displayName.indexOf( "(" ); return i >= 0 ? displayName.substring( 0, i ) : displayName; }}
package org.apache.maven.surefire.util.internal;

/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

import java.util.regex.Matcher;
import java.util.regex.Pattern;

/**
* JUnit Description parser.
* Used by JUnit Version lower than 4.7.
*
* @author <a href="mailto:tibordigana@apache.org">Tibor Digana (tibor17)</a>
* @since 2.20
*/
public final class TestClassMethodNameUtils
{
private static final Pattern METHOD_CLASS_PATTERN = Pattern.compile( "([\\s\\S]*)\\((.*)\\)" );

This comment has been minimized.

Copy link
@michael-o

michael-o May 4, 2017

Member

That's a weird pattern: \s\S*. That's basically .*. Is that really correct?

This comment has been minimized.

Copy link
@Tibor17

Tibor17 May 4, 2017

Contributor

This pattern is copy from JUnit source. I told myself the same that it is very similar to .* but I used same from JUnit because I am sure the pattern gives identical result as JUnit returns.
The point is that JUnit4's Description has method getDisplayName() method which encodes method(class) in one string. The version 4.7+ is parsing class and method name via methods getClassName() and getMethodName(). We need to have them in both Surefire providers (surefire-junit and surefire-junit47) behaving the same.
Should I change the pattern?

This comment has been minimized.

Copy link
@michael-o

michael-o May 4, 2017

Member

No, leave it as it. Just place a comment that this pattern comes from JUnit and has been verbatimly copied. It caused confusion here and will cause it somewhere else.

This comment has been minimized.

Copy link
@Tibor17

Tibor17 May 4, 2017

Contributor

@michael-o Done.

This comment has been minimized.

Copy link
@michael-o

michael-o May 4, 2017

Member

OK, go ahead and merge.


private TestClassMethodNameUtils()
{
throw new IllegalStateException( "no instantiable constructor" );
}

public static String extractClassName( String displayName )
{
Matcher m = METHOD_CLASS_PATTERN.matcher( displayName );
return m.matches() ? m.group( 2 ) : displayName;
}

public static String extractMethodName( String displayName )
{
Matcher m = METHOD_CLASS_PATTERN.matcher( displayName );
return m.matches() ? m.group( 1 ) : null;
}
}
Expand Up @@ -39,7 +39,6 @@
*/
public class TestFile
{

private final File file;

private final Charset encoding;
Expand Down Expand Up @@ -148,4 +147,9 @@ public URI toURI()
{
return file.toURI();
}

public File getFile()
{
return file;
}
}
Expand Up @@ -23,17 +23,22 @@
import org.apache.maven.surefire.its.fixture.OutputValidator;
import org.apache.maven.surefire.its.fixture.SurefireJUnit4IntegrationTestCase;
import org.apache.maven.surefire.its.fixture.SurefireLauncher;
import org.apache.maven.surefire.its.fixture.TestFile;
import org.hamcrest.BaseMatcher;
import org.hamcrest.Description;
import org.hamcrest.Matcher;
import org.junit.Test;

import java.nio.charset.Charset;
import java.util.Collection;
import java.util.Iterator;
import java.util.Set;
import java.util.SortedSet;
import java.util.TreeSet;

import static org.hamcrest.core.AnyOf.anyOf;
import static org.hamcrest.core.Is.is;
import static org.hamcrest.core.StringContains.containsString;
import static org.junit.Assert.assertThat;

/**
Expand All @@ -44,17 +49,29 @@
public class Surefire1082ParallelJUnitParameterizedIT
extends SurefireJUnit4IntegrationTestCase
{
private static Set<String> printOnlyTestLinesFromConsole( OutputValidator validator )
throws VerificationException
{
return printOnlyTestLines( validator.loadLogLines() );
}

private static Set<String> printOnlyTestLinesFromOutFile( OutputValidator validator )
throws VerificationException
{
TestFile report = validator.getSurefireReportsFile( "jiras.surefire1082.Jira1082Test-output.txt" );
report.assertFileExists();
return printOnlyTestLines( validator.loadFile( report.getFile(), Charset.forName( "UTF-8" ) ) );
}

private static Set<String> printOnlyTestLines( OutputValidator validator )
private static Set<String> printOnlyTestLines( Collection<String> logs )
throws VerificationException
{
Set<String> log = new TreeSet<String>( validator.loadLogLines() );
for ( Iterator<String> it = log.iterator(); it.hasNext(); )
SortedSet<String> log = new TreeSet<String>();

This comment has been minimized.

Copy link
@michael-o

michael-o May 4, 2017

Member

Why not stay with Set instead of SortedSet?

This comment has been minimized.

Copy link
@Tibor17

Tibor17 May 4, 2017

Contributor

This is older code. I think the behavior would not mind at all. The only which matters is the result if assertion statement fails in this junit/Hamcrest code. This is parallel execution of tests and thus the lines in log are somehow randomly ordered. Asserting the Set satisfies [1] regex, the strings are very similar and hard to see a slight one char difference in assertion failure. So therefore I think, that time, decided to order the strings in logs as literals and better to see then the differences in assertions when failed.
[1]
"class jiras.surefire1082.Jira1082Test a 1 pool-[\d]+-thread-1"
"class jiras.surefire1082.Jira1082Test b 1 pool-[\d]+-thread-1"
etc.

This comment has been minimized.

Copy link
@michael-o

michael-o May 4, 2017

Member

I wasn't arguing about sorting, but solely asking why you supplied a sub-interface where Set is enough. The implementation TreeSet won't change, so behavior won't.

This comment has been minimized.

Copy link
@Tibor17

Tibor17 May 4, 2017

Contributor

I will override it to Set interface.

This comment has been minimized.

Copy link
@Tibor17

Tibor17 May 4, 2017

Contributor

@michael-o Done.

for ( String line : logs )
{
String line = it.next();
if ( !line.startsWith( "class jiras.surefire1082." ) )
if ( line.startsWith( "class jiras.surefire1082." ) )
{
it.remove();
log.add( line );
}
}
return log;
Expand All @@ -65,14 +82,8 @@ private static Matcher<Set<String>> regex( Set<String> r )
return new IsRegex( r );
}

@Test
public void test()
throws VerificationException
private static void assertParallelRun( Set<String> log )
{
OutputValidator validator = unpack().setTestToRun(
"Jira1082Test" ).parallelClasses().useUnlimitedThreads().executeTest().verifyErrorFree( 4 );

Set<String> log = printOnlyTestLines( validator );
assertThat( log.size(), is( 4 ) );

Set<String> expectedLogs1 = new TreeSet<String>();
Expand All @@ -90,6 +101,71 @@ public void test()
assertThat( log, anyOf( regex( expectedLogs1 ), regex( expectedLogs2 ) ) );
}

@Test
public void checkClassesRunParallel()
throws VerificationException
{
OutputValidator validator = unpack().setTestToRun( "Jira1082Test" )
.parallelClasses()
.useUnlimitedThreads()
.executeTest()
.verifyErrorFree( 4 );

validator.getSurefireReportsXmlFile( "TEST-jiras.surefire1082.Jira1082Test.xml" )
.assertFileExists();

validator.assertThatLogLine( containsString( "Running jiras.surefire1082.Jira1082Test" ), is( 1 ) );

Set<String> log = printOnlyTestLinesFromConsole( validator );
assertParallelRun( log );
}

@Test
public void checkOutFileClassesRunParallel()
throws VerificationException
{
OutputValidator validator = unpack().redirectToFile( true )
.setTestToRun( "Jira1082Test" )
.parallelClasses()
.useUnlimitedThreads()
.executeTest()
.verifyErrorFree( 4 );

validator.getSurefireReportsXmlFile( "TEST-jiras.surefire1082.Jira1082Test.xml" )
.assertFileExists();

validator.assertThatLogLine( containsString( "Running jiras.surefire1082.Jira1082Test" ), is( 1 ) );

Set<String> log = printOnlyTestLinesFromOutFile( validator );
assertParallelRun( log );
}

@Test
public void shouldRunTwo() throws VerificationException
{
OutputValidator validator = unpack().redirectToFile( true )
.parallelClasses()
.useUnlimitedThreads()
.executeTest()
.verifyErrorFree( 8 );

validator.getSurefireReportsXmlFile( "TEST-jiras.surefire1082.Jira1082Test.xml" )
.assertFileExists();

validator.getSurefireReportsXmlFile( "TEST-jiras.surefire1082.Jira1264Test.xml" )
.assertFileExists();

validator.getSurefireReportsFile( "jiras.surefire1082.Jira1082Test-output.txt" )
.assertFileExists();

validator.getSurefireReportsFile( "jiras.surefire1082.Jira1264Test-output.txt" )
.assertFileExists();

validator.assertThatLogLine( containsString( "Running jiras.surefire1082.Jira1082Test" ), is( 1 ) );

validator.assertThatLogLine( containsString( "Running jiras.surefire1082.Jira1264Test" ), is( 1 ) );
}

private SurefireLauncher unpack()
{
return unpack( "surefire-1082-parallel-junit-parameterized" );
Expand Down
Expand Up @@ -27,9 +27,9 @@
import java.util.concurrent.TimeUnit;

@RunWith( Parameterized.class )
public final class Jira1082Test
public class Jira1082Test
{
private int x;
private final int x;

public Jira1082Test( int x )
{
Expand Down
@@ -0,0 +1,36 @@
package jiras.surefire1082;

/*
* Licensed to the Apache Software Foundation (ASF) under one
* or more contributor license agreements. See the NOTICE file
* distributed with this work for additional information
* regarding copyright ownership. The ASF licenses this file
* to you under the Apache License, Version 2.0 (the
* "License"); you may not use this file except in compliance
* with the License. You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing,
* software distributed under the License is distributed on an
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
* KIND, either express or implied. See the License for the
* specific language governing permissions and limitations
* under the License.
*/

import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;

import java.util.Arrays;
import java.util.concurrent.TimeUnit;

@RunWith( Parameterized.class )
public final class Jira1264Test extends Jira1082Test
{
public Jira1264Test( int x )
{
super( x );
}
}
Expand Up @@ -19,16 +19,18 @@
* under the License.
*/

import java.util.ArrayList;
import java.util.Map;
import org.apache.maven.surefire.common.junit4.JUnit4RunListener;
import org.apache.maven.surefire.common.junit48.JUnit46StackTraceWriter;
import org.apache.maven.surefire.report.RunListener;
import org.apache.maven.surefire.report.StackTraceWriter;

import org.junit.runner.Description;
import org.junit.runner.Result;
import org.junit.runner.RunWith;
import org.junit.runner.notification.Failure;
import org.junit.runners.Parameterized;

import java.lang.annotation.Annotation;
import java.util.Map;

/**
* Noteworthy things about JUnit4 listening:
Expand Down Expand Up @@ -74,38 +76,57 @@ public void testRunFinished( Result result )
reporter.testSetCompleted( null );
}

private void fillTestCountMap( Description description )
private void fillTestCountMap( Description testDesc )
{
final ArrayList<Description> children = description.getChildren();
TestSet testSet = new TestSet( testDesc );

String itemTestClassName =
isParameterizedRunner( testDesc ) ? testDesc.getClassName() : asSuiteRunner( testDesc, testSet );

TestSet testSet = new TestSet( description );
if ( itemTestClassName != null )
{
classMethodCounts.put( itemTestClassName, testSet );
}
}

private String asSuiteRunner( Description description, TestSet testSet )
{
String itemTestClassName = null;
for ( Description item : children )
for ( Description child : description.getChildren() )
{
if ( !item.isTest() )
if ( !child.isTest() )
{
fillTestCountMap( item );
fillTestCountMap( child );
}
else
{
if ( extractDescriptionMethodName( item ) != null )
if ( extractDescriptionMethodName( child ) != null )
{
testSet.incrementTestMethodCount();
if ( itemTestClassName == null )
{
itemTestClassName = extractDescriptionClassName( item );
itemTestClassName = extractDescriptionClassName( child );
}
}
else
{
classMethodCounts.put( extractDescriptionClassName( item ), new TestSet( item ) );
classMethodCounts.put( extractDescriptionClassName( child ), new TestSet( child ) );
}
}
}
if ( itemTestClassName != null )
return itemTestClassName;
}

private static boolean isParameterizedRunner( Description description )
{
for ( Annotation ann : description.getAnnotations() )
{
classMethodCounts.put( itemTestClassName, testSet );
if ( ann.annotationType() == RunWith.class )
{
return Parameterized.class.isAssignableFrom( ( (RunWith) ann ).value() );
}
}
return false;
}

@Override
Expand Down

1 comment on commit 92b39d6

@michael-o
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Almost fine expect a small few things.

Please sign in to comment.