From 8526baf60f622943330780f7dfffa4636be590b3 Mon Sep 17 00:00:00 2001 From: Andreas Schmid Date: Fri, 30 May 2014 13:13:38 +0200 Subject: [PATCH] replaced blacklisting with always wrapped filter which fallbacks to original filter (#27) --- README.md | 1 + .../dataprovider/DataProviderFilter.java | 40 ++++++------ .../dataprovider/DataProviderRunner.java | 42 +++---------- .../dataprovider/DataProviderFilterTest.java | 63 +++++++++++-------- .../dataprovider/DataProviderRunnerTest.java | 57 +++++------------ 5 files changed, 81 insertions(+), 122 deletions(-) diff --git a/README.md b/README.md index d0e79e23..bd069e2e 100644 --- a/README.md +++ b/README.md @@ -325,6 +325,7 @@ Release notes ### tbd. (???) +* more fault tolerant filtering instead of explicitly maintain a black or white list ([#27](/../../issues/27)) * ... ### v1.7.0 (20-Jun-2014) diff --git a/src/main/java/com/tngtech/java/junit/dataprovider/DataProviderFilter.java b/src/main/java/com/tngtech/java/junit/dataprovider/DataProviderFilter.java index 5bd3f9d7..c2fc647a 100644 --- a/src/main/java/com/tngtech/java/junit/dataprovider/DataProviderFilter.java +++ b/src/main/java/com/tngtech/java/junit/dataprovider/DataProviderFilter.java @@ -24,43 +24,41 @@ public class DataProviderFilter extends Filter { private static final int GROUP_METHOD_IDX = 3; private static final int GROUP_CLASS = 4; - private final String filterDescription; - private final Matcher filterDescriptionMatcher; + /** + * Original filter which is used if its description (= {@link Filter#describe()}) is not parsable by + * {@link #DESCRIPTION_PATTERN} + *

+ * This field is package private (= visible) for testing. + *

+ **/ + final Filter filter; /** - * Creates a new {@link DataProvider} using the textual {@link Filter#describe()} of supplied {@link Filter} to - * determine if a test method should run or not. - * - * @throws IllegalArgumentException if supplied {@link Filter} is {@code null} or - * {@link Description#getDisplayName()} of supplied {@link Description} cannot be parsed + * Creates a new {@link DataProviderFilter} using the textual {@link Filter#describe()} of supplied {@link Filter} + * to determine if a test method should run or not. If given {@code filter} description can not be parsed, request + * for {@link #shouldRun(Description)} are just forwarded to it. */ public DataProviderFilter(Filter filter) { if (filter == null) { throw new NullPointerException("supplied filter must not be null"); } - filterDescription = filter.describe(); - filterDescriptionMatcher = DESCRIPTION_PATTERN.matcher(filterDescription); - if (!filterDescriptionMatcher.find()) { - throw new IllegalArgumentException(String.format("Filter %s with description %s is not supported by %s.", - filter.getClass(), filterDescription, this.getClass().getSimpleName())); - } + this.filter = filter; } - /** - * @throws IllegalArgumentException if {@link Description#getDisplayName()} of supplied {@link Description} cannot - * be parsed - */ @Override public boolean shouldRun(Description description) { + Matcher filterDescriptionMatcher = DESCRIPTION_PATTERN.matcher(filter.describe()); + if (!filterDescriptionMatcher.find()) { + return filter.shouldRun(description); + } + if (description.isTest()) { Matcher descriptionMatcher = DESCRIPTION_PATTERN.matcher(description.getDisplayName()); if (!descriptionMatcher.matches()) { - throw new IllegalArgumentException(String.format("Test method description %s is not suppored by %s.", - description.getDisplayName(), this.getClass().getSimpleName())); + return filter.shouldRun(description); } if (!filterDescriptionMatcher.group(GROUP_METHOD_NAME).equals(descriptionMatcher.group(GROUP_METHOD_NAME)) || !filterDescriptionMatcher.group(GROUP_CLASS).equals(descriptionMatcher.group(GROUP_CLASS))) { - return false; } return filterDescriptionMatcher.group(GROUP_METHOD_PARAMS) == null @@ -79,6 +77,6 @@ public boolean shouldRun(Description description) { @Override public String describe() { - return filterDescription; + return filter.describe(); } } diff --git a/src/main/java/com/tngtech/java/junit/dataprovider/DataProviderRunner.java b/src/main/java/com/tngtech/java/junit/dataprovider/DataProviderRunner.java index 5067799d..e3e075ae 100644 --- a/src/main/java/com/tngtech/java/junit/dataprovider/DataProviderRunner.java +++ b/src/main/java/com/tngtech/java/junit/dataprovider/DataProviderRunner.java @@ -1,11 +1,9 @@ package com.tngtech.java.junit.dataprovider; import java.util.ArrayList; -import java.util.Arrays; import java.util.List; import org.junit.Test; -import org.junit.experimental.categories.Categories.CategoryFilter; import org.junit.runner.manipulation.Filter; import org.junit.runner.manipulation.NoTestsRemainException; import org.junit.runners.BlockJUnit4ClassRunner; @@ -26,16 +24,6 @@ */ public class DataProviderRunner extends BlockJUnit4ClassRunner { - /** - * A list of filter packages which must not be wrapped by DataProviderRunner (this is a workaround for some plugins, - * e.g. the maven-surefire-plugin). - */ - // @formatter:off - private static final List BLACKLISTED_FILTER_PACKAGES = Arrays.asList( - "org.apache.maven.surefire" - ); - // @formatter:on - /** * The {@link DataConverter} to be used to convert from supported return types of any data provider to {@link List} * {@code <}{@link Object}{@code []>} such that data can be further handled. @@ -152,17 +140,16 @@ protected List computeTestMethods() { * If possible the given {@code filter} is wrapped by {@link DataProviderFilter} to enable filtering of tests using * a data provider. * - * @param filter the {@link Filter} to wrap/apply + * @param filter the {@link Filter} to be wrapped or apply, respectively */ @Override public void filter(Filter filter) throws NoTestsRemainException { - Filter useFilter; - if (!(filter instanceof CategoryFilter) && !isFilterBlackListed(filter)) { - useFilter = new DataProviderFilter(filter); - } else { - useFilter = filter; - } - super.filter(useFilter); + // Filter newFilter = filter; + // // TODO not really testable? maybe first create and than test if it is applicable? + // if (DataProviderFilter.canBeWrapped(filter)) { + // newFilter = ; + // } + super.filter(new DataProviderFilter(filter)); } /** @@ -197,21 +184,6 @@ List generateExplodedTestMethodsFor(List testM return result; } - /** - *

- * This method is package private (= visible) for testing. - *

- */ - boolean isFilterBlackListed(Filter filter) { - String className = filter.getClass().getName(); - for (String blacklistedPackage : BLACKLISTED_FILTER_PACKAGES) { - if (className.startsWith(blacklistedPackage)) { - return true; - } - } - return false; - } - /** * Returns the data provider method that belongs to the given test method or {@code null} if no such data provider * exists or the test method is not marked for usage of a data provider diff --git a/src/test/java/com/tngtech/java/junit/dataprovider/DataProviderFilterTest.java b/src/test/java/com/tngtech/java/junit/dataprovider/DataProviderFilterTest.java index c602b665..22bd6cfd 100644 --- a/src/test/java/com/tngtech/java/junit/dataprovider/DataProviderFilterTest.java +++ b/src/test/java/com/tngtech/java/junit/dataprovider/DataProviderFilterTest.java @@ -3,6 +3,8 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyNoMoreInteractions; import java.util.ArrayList; import java.util.Arrays; @@ -10,12 +12,19 @@ import org.junit.Test; import org.junit.runner.Description; +import org.junit.runner.RunWith; import org.junit.runner.manipulation.Filter; +import org.mockito.InjectMocks; +import org.mockito.Mock; +import org.mockito.runners.MockitoJUnitRunner; +@RunWith(MockitoJUnitRunner.class) public class DataProviderFilterTest extends BaseTest { + @InjectMocks private DataProviderFilter underTest; + @Mock private Filter filter; @edu.umd.cs.findbugs.annotations.SuppressWarnings("DLS_DEAD_LOCAL_STORE") @@ -30,32 +39,40 @@ public void testDataProviderFilterShouldThrowNullPointerExceptionWhenFilterIsNul // Then: expect exception } - @Test(expected = IllegalArgumentException.class) - public void testDataProviderFilterShouldThrowIllegalArgumentExceptionWhenFilterDescriptionCannotBeParsed() { + @Test + public void testShouldRunShouldCallOriginalFilterShouldRunIfOriginalFilterDescriptionCannotBeParsed() { // Given: + doReturn("invalid").when(filter).describe(); + Description description = setupDescription(true, "test(Clazz)"); // When: - setupDataProviderFilterWith("invalid"); + underTest.shouldRun(description); - // Then: expect exception + // Then: + verify(filter).describe(); + verify(filter).shouldRun(description); + verifyNoMoreInteractions(filter); } - @Test(expected = IllegalArgumentException.class) - public void testShouldRunShouldThrowIllegalArgumentExceptionWhenDescriptionCannotBeParsed() { + @Test + public void testShouldRunShouldCallOriginalFilterShouldRunIfIsTestAndGivenDescriptionCannotBeParsed() { // Given: - setupDataProviderFilterWith("Method testMain[1: ](Clazz)"); + doReturn("Method test(Clazz)").when(filter).describe(); Description description = setupDescription(true, "invalid"); // When: underTest.shouldRun(description); - // Then: expect exception + // Then: + verify(filter).describe(); + verify(filter).shouldRun(description); + verifyNoMoreInteractions(filter); } @Test public void testShouldRunShouldReturnFalseWhenDescriptionDoesNotHaveExpectedMethodName() { // Given: - setupDataProviderFilterWith("Method testMain[1: ](Clazz)"); + doReturn("Method testMain[1: ](Clazz)").when(filter).describe(); Description description = setupDescription(true, "testOther[1: ](Clazz)"); // When: @@ -68,7 +85,7 @@ public void testShouldRunShouldReturnFalseWhenDescriptionDoesNotHaveExpectedMeth @Test public void testShouldRunShouldReturnFalseWhenDescriptionDoesNotHaveExpectedClassName() { // Given: - setupDataProviderFilterWith("Method testMain[1: ](Clazz)"); + doReturn("Method testMain[1: ](Clazz)").when(filter).describe(); Description description = setupDescription(true, "testMain[1: ](ClazzOther)"); // When: @@ -81,7 +98,7 @@ public void testShouldRunShouldReturnFalseWhenDescriptionDoesNotHaveExpectedClas @Test public void testShouldRunShouldReturnFalseWhenDescriptionHasNoMethodIdx() { // Given: - setupDataProviderFilterWith("Method testMain[1: ](Clazz)"); + doReturn("Method testMain[1: ](Clazz)").when(filter).describe(); Description description = setupDescription(true, "testMain(Clazz)"); // When: @@ -94,7 +111,7 @@ public void testShouldRunShouldReturnFalseWhenDescriptionHasNoMethodIdx() { @Test public void testShouldRunShouldReturnFalseWhenDescriptionDoesNotHaveExpectedMethodIdx() { // Given: - setupDataProviderFilterWith("Method testMain[1: ](Clazz)"); + doReturn("Method testMain[1: ](Clazz)").when(filter).describe(); Description description = setupDescription(true, "testMain[2: ](Clazz)"); // When: @@ -107,7 +124,7 @@ public void testShouldRunShouldReturnFalseWhenDescriptionDoesNotHaveExpectedMeth @Test public void testShouldRunShouldReturnTrueWhenDescriptionHaveOnlyMethodNameAndEqualsExactly() { // Given: - setupDataProviderFilterWith("Method testMain(Clazz)"); + doReturn("Method testMain(Clazz)").when(filter).describe(); Description description = setupDescription(true, "testMain(Clazz)"); // When: @@ -120,7 +137,7 @@ public void testShouldRunShouldReturnTrueWhenDescriptionHaveOnlyMethodNameAndEqu @Test public void testShouldRunShouldReturnTrueWhenDescriptionHaveAdditionalMethodIdxAndEqualsMethodNameAndClass() { // Given: - setupDataProviderFilterWith("Method testMain(Clazz)"); + doReturn("Method testMain(Clazz)").when(filter).describe(); Description description = setupDescription(true, "testMain[1: ](Clazz)"); // When: @@ -133,7 +150,7 @@ public void testShouldRunShouldReturnTrueWhenDescriptionHaveAdditionalMethodIdxA @Test public void testShouldRunShouldReturnTrueWhenDescriptionHaveAddtionalMethodIdxAndEqualsExcatly() { // Given: - setupDataProviderFilterWith("Method testMain[1: ](Clazz)"); + doReturn("Method testMain[1: ](Clazz)").when(filter).describe(); Description description = setupDescription(true, "testMain[1: ](Clazz)"); // When: @@ -146,7 +163,7 @@ public void testShouldRunShouldReturnTrueWhenDescriptionHaveAddtionalMethodIdxAn @Test public void testShouldRunShouldReturnTrueWhenDescriptionHaveAdditionalMethodIdxAndMethodParamsAreDifferentButIdxIsEqual() { // Given: - setupDataProviderFilterWith("Method testMain[1: ](Clazz)"); + doReturn("Method testMain[1: ](Clazz)").when(filter).describe(); Description description = setupDescription(true, "testMain[1: test](Clazz)"); // When: @@ -159,7 +176,7 @@ public void testShouldRunShouldReturnTrueWhenDescriptionHaveAdditionalMethodIdxA @Test public void testShouldRunShouldReturnTrueForMatchingChildDescription() { // Given: - setupDataProviderFilterWith("Method testMain[1: ](Clazz)"); + doReturn("Method testMain[1: ](Clazz)").when(filter).describe(); Description description = setupDescription(false, "", setupDescription(true, "testMain[1: ](Clazz)")); @@ -173,7 +190,7 @@ public void testShouldRunShouldReturnTrueForMatchingChildDescription() { @Test public void testShouldRunShouldReturnTrueForMultipleChildDescriptionWithLastMatching() { // Given: - setupDataProviderFilterWith("Method testMain[1: ](Clazz)"); + doReturn("Method testMain[1: ](Clazz)").when(filter).describe(); // @formatter:off Description description = setupDescription(false, "", @@ -193,7 +210,7 @@ public void testShouldRunShouldReturnTrueForMultipleChildDescriptionWithLastMatc @Test public void testShouldRunShouldReturnFalseForMultipleChildAndFurtherChildDescriptionWithNonMatching() { // Given: - setupDataProviderFilterWith("Method testMain[1: ](Clazz)"); + doReturn("Method testMain[1: ](Clazz)").when(filter).describe(); // @formatter:off Description description = setupDescription(false, "testMain[2: ](Clazz)", @@ -217,7 +234,7 @@ public void testShouldRunShouldReturnFalseForMultipleChildAndFurtherChildDescrip @Test public void testDescribeShouldReturnFilterDescripe() { // Given: - setupDataProviderFilterWith("Method testMain[1: ](Clazz)"); + doReturn("Method testMain[1: ](Clazz)").when(filter).describe(); // When: String result = underTest.describe(); @@ -326,12 +343,6 @@ public void testDescriptionPatternShouldFindDescriptionWithMethodNameContainingB assertThatMatcherGroupsAre(matcher, "ain", null, null, "Clazz"); } - private void setupDataProviderFilterWith(String filterDescriptionString) { - filter = mock(Filter.class); - doReturn(filterDescriptionString).when(filter).describe(); - underTest = new DataProviderFilter(filter); - } - private Description setupDescription(boolean isTest, String descriptionDisplayName, Description... childDescriptions) { diff --git a/src/test/java/com/tngtech/java/junit/dataprovider/DataProviderRunnerTest.java b/src/test/java/com/tngtech/java/junit/dataprovider/DataProviderRunnerTest.java index e842cd6a..5cece528 100644 --- a/src/test/java/com/tngtech/java/junit/dataprovider/DataProviderRunnerTest.java +++ b/src/test/java/com/tngtech/java/junit/dataprovider/DataProviderRunnerTest.java @@ -9,15 +9,14 @@ import static org.mockito.Mockito.verifyNoMoreInteractions; import static org.mockito.Mockito.verifyZeroInteractions; +import java.lang.reflect.Field; import java.util.ArrayList; import java.util.List; import org.junit.Before; import org.junit.Test; -import org.junit.experimental.categories.Categories; -import org.junit.runner.Description; import org.junit.runner.manipulation.Filter; -import org.junit.runner.manipulation.NoTestsRemainException; +import org.junit.runners.ParentRunner; import org.junit.runners.model.FrameworkMethod; import org.junit.runners.model.TestClass; import org.mockito.Mock; @@ -27,7 +26,6 @@ import com.tngtech.java.junit.dataprovider.internal.DataConverter; import com.tngtech.java.junit.dataprovider.internal.TestGenerator; import com.tngtech.java.junit.dataprovider.internal.TestValidator; -import com.tngtech.test.java.junit.dataprovider.category.CategoryOne; public class DataProviderRunnerTest extends BaseTest { @@ -236,36 +234,19 @@ public void testComputeTestMethodsShouldNotCallGenerateExplodedTestMethodsAndUse } @Test - public void testFilterShouldNotThrowExceptionForJUnitCategoryFilter() throws Exception { + public void testFilterShouldWrapGivenFilterWithDataProviderFilter() throws Exception { // Given: - Filter filter = new Categories.CategoryFilter(null, CategoryOne.class); + Filter filter = Filter.ALL; // When: underTest.filter(filter); - // Then: expect no exception - } - - @Test(expected = NoTestsRemainException.class) - public void testFilterShouldThrowNoTestRemainExceptionForNonBlacklistedAndRecognizableFilterHavingNoTestMethods() - throws Exception { - // Given: - Filter filter = new Filter() { - @Override - public boolean shouldRun(Description description) { - return true; - } - - @Override - public String describe() { - return "testMethod(com.tngtech.java.junit.dataprovider.Test)"; - } - }; - - // When: - underTest.filter(filter); + // Then: + Object actual = getPrivateField(ParentRunner.class, "fFilter", underTest); + assertThat(actual).isInstanceOf(DataProviderFilter.class); - // Then: expect no exception + Object actualFilter = getPrivateField(DataProviderFilter.class, "filter", actual); + assertThat(actualFilter).isEqualTo(filter); } @Test @@ -361,18 +342,6 @@ public void testGenerateExplodedTestMethodsForShouldCallFrameworkMethodGenerator verifyNoMoreInteractions(frameworkMethodGenerator); } - @Test - public void testIsFilterBlackListedShouldReturnFalseForJunitPackagedFilter() { - // Given: - Filter filter = Filter.ALL; - - // When: - boolean result = underTest.isFilterBlackListed(filter); - - // Then: - assertThat(result).isFalse(); - } - @Test(expected = IllegalArgumentException.class) public void testGetDataProviderMethodShouldThrowIllegalArgumentExceptionIfTestMethodIsNull() { // Given: @@ -457,4 +426,12 @@ public void testFindDataProviderLocationShouldReturnTestClassContainingSetLocati // assertThat(result.getJavaClass()).isEqualTo(dataProviderLocation); assertThat(result.getName()).isEqualTo(dataProviderLocation.getName()); } + + // -- helper methods ----------------------------------------------------------------------------------------------- + + private Object getPrivateField(Class clazz, String fieldName, Object instance) throws Exception { + Field filterField = clazz.getDeclaredField(fieldName); + filterField.setAccessible(true); + return filterField.get(instance); + } }