Skip to content

Commit

Permalink
Ensure JUnit Jupiter test classes cannot be private
Browse files Browse the repository at this point in the history
Prior to this commit, if a test class was discovered via package, class
path, or module path scanning, it could NOT be private; however, if the
same test class was selected explicitly (i.e., via one of the
`selectClass()` variants in `DiscoverySelectors`, via the `aClass` or
`classes` element of the `selectors` configuration for the
`junitPlatform` Gradle plugin, or via the `-select-class` or `-c`
command-line options for the `ConsoleLauncher`) it COULD then be
private.

This behavior is inconsistent and dangerous: although a developer could
execute such a test class in an IDE, the same test classes would never
be executed with a typical build configuration that relies on class
path or module path scanning.

This commit ensures that JUnit Jupiter test classes can never be
private, regardless of how they are discovered or selected.

In addition, this commit removes the now obsolete IsScannableTestClass
predicate since scanned test classes are no longer handled differently.

Fixes: #1252
  • Loading branch information
sbrannen committed Jan 19, 2018
1 parent 3c57299 commit e4789f3
Show file tree
Hide file tree
Showing 81 changed files with 466 additions and 494 deletions.
Expand Up @@ -77,7 +77,11 @@ _@API Guardian_ JAR _mandatory_ again.


==== Bug Fixes ==== Bug Fixes


* ❓ * Test classes selected via one of the `selectClass()` variants in `DiscoverySelectors`,
via the `aClass` or `classes` element of the `selectors` configuration for the
`junitPlatform` Gradle plugin, or via the `-select-class` or `-c` command-line options
for the `ConsoleLauncher` are no longer allowed to be `private`. This aligns with the
behavior for test classes discovered via package, class path, and module path scanning.


==== Deprecations and Breaking Changes ==== Deprecations and Breaking Changes


Expand Down
Expand Up @@ -20,7 +20,7 @@
import java.util.Set; import java.util.Set;


import org.apiguardian.api.API; import org.apiguardian.api.API;
import org.junit.jupiter.engine.discovery.predicates.IsScannableTestClass; import org.junit.jupiter.engine.discovery.predicates.IsTestClassWithTests;
import org.junit.platform.commons.util.ClassFilter; import org.junit.platform.commons.util.ClassFilter;
import org.junit.platform.engine.EngineDiscoveryRequest; import org.junit.platform.engine.EngineDiscoveryRequest;
import org.junit.platform.engine.TestDescriptor; import org.junit.platform.engine.TestDescriptor;
Expand All @@ -43,10 +43,10 @@
@API(status = INTERNAL, since = "5.0") @API(status = INTERNAL, since = "5.0")
public class DiscoverySelectorResolver { public class DiscoverySelectorResolver {


private static final IsScannableTestClass isScannableTestClass = new IsScannableTestClass(); private static final IsTestClassWithTests isTestClassWithTests = new IsTestClassWithTests();


public void resolveSelectors(EngineDiscoveryRequest request, TestDescriptor engineDescriptor) { public void resolveSelectors(EngineDiscoveryRequest request, TestDescriptor engineDescriptor) {
ClassFilter classFilter = buildClassFilter(request, isScannableTestClass); ClassFilter classFilter = buildClassFilter(request, isTestClassWithTests);
resolve(request, engineDescriptor, classFilter); resolve(request, engineDescriptor, classFilter);
filter(engineDescriptor, classFilter); filter(engineDescriptor, classFilter);
pruneTree(engineDescriptor); pruneTree(engineDescriptor);
Expand Down
Expand Up @@ -12,6 +12,7 @@


import static org.apiguardian.api.API.Status.INTERNAL; import static org.apiguardian.api.API.Status.INTERNAL;
import static org.junit.platform.commons.util.ReflectionUtils.isAbstract; import static org.junit.platform.commons.util.ReflectionUtils.isAbstract;
import static org.junit.platform.commons.util.ReflectionUtils.isPrivate;
import static org.junit.platform.commons.util.ReflectionUtils.isStatic; import static org.junit.platform.commons.util.ReflectionUtils.isStatic;


import java.util.function.Predicate; import java.util.function.Predicate;
Expand All @@ -29,7 +30,10 @@ public class IsPotentialTestContainer implements Predicate<Class<?>> {


@Override @Override
public boolean test(Class<?> candidate) { public boolean test(Class<?> candidate) {
//please do not collapse into single return // Please do not collapse the following into a single statement.
if (isPrivate(candidate)) {
return false;
}
if (isAbstract(candidate)) { if (isAbstract(candidate)) {
return false; return false;
} }
Expand Down

This file was deleted.

Expand Up @@ -10,18 +10,22 @@


package org.junit.jupiter.engine.discovery.predicates; package org.junit.jupiter.engine.discovery.predicates;


import static org.apiguardian.api.API.Status.INTERNAL;

import java.lang.reflect.Method; import java.lang.reflect.Method;
import java.util.function.Predicate; import java.util.function.Predicate;


import org.apiguardian.api.API;
import org.junit.platform.commons.util.ReflectionUtils; import org.junit.platform.commons.util.ReflectionUtils;


/** /**
* Test if a class is a JUnit Jupiter test class containing executable tests, * Test if a class is a JUnit Jupiter test class containing executable tests,
* test factories, or nested tests. * test factories, test templates, or nested tests.
* *
* @since 5.0 * @since 5.0
*/ */
class IsTestClassWithTests implements Predicate<Class<?>> { @API(status = INTERNAL, since = "5.1")
public class IsTestClassWithTests implements Predicate<Class<?>> {


private static final IsTestMethod isTestMethod = new IsTestMethod(); private static final IsTestMethod isTestMethod = new IsTestMethod();


Expand All @@ -38,11 +42,8 @@ class IsTestClassWithTests implements Predicate<Class<?>> {


@Override @Override
public boolean test(Class<?> candidate) { public boolean test(Class<?> candidate) {
// please do not collapse into single return return isPotentialTestContainer.test(candidate)
if (!isPotentialTestContainer.test(candidate)) { && (hasTestOrTestFactoryOrTestTemplateMethods(candidate) || hasNestedTests(candidate));
return false;
}
return hasTestOrTestFactoryOrTestTemplateMethods(candidate) || hasNestedTests(candidate);
} }


private boolean hasTestOrTestFactoryOrTestTemplateMethods(Class<?> candidate) { private boolean hasTestOrTestFactoryOrTestTemplateMethods(Class<?> candidate) {
Expand Down
Expand Up @@ -43,7 +43,7 @@ void beforeAllAndAfterAllAsMetaAnnotations() {
assertEquals(asList("beforeAll", "test", "afterAll"), methodsInvoked); assertEquals(asList("beforeAll", "test", "afterAll"), methodsInvoked);
} }


private static class TestCase { static class TestCase {


@CustomBeforeAll @CustomBeforeAll
static void beforeAll() { static void beforeAll() {
Expand Down
Expand Up @@ -43,7 +43,7 @@ void beforeEachAndAfterEachAsMetaAnnotations() {
assertEquals(asList("beforeEach", "test", "afterEach"), methodsInvoked); assertEquals(asList("beforeEach", "test", "afterEach"), methodsInvoked);
} }


private static class TestCase { static class TestCase {


@CustomBeforeEach @CustomBeforeEach
void beforeEach() { void beforeEach() {
Expand Down
Expand Up @@ -53,15 +53,15 @@ void executeTestsWithDisabledTestMethods() throws Exception {
// ------------------------------------------------------------------- // -------------------------------------------------------------------


@Disabled @Disabled
private static class DisabledTestClassTestCase { static class DisabledTestClassTestCase {


@Test @Test
void disabledTest() { void disabledTest() {
fail("this should be @Disabled"); fail("this should be @Disabled");
} }
} }


private static class DisabledTestMethodsTestCase { static class DisabledTestMethodsTestCase {


@Test @Test
void enabledTest() { void enabledTest() {
Expand Down
Expand Up @@ -190,7 +190,7 @@ public void cleanUpExceptions() {
FailureTestCase.exceptionToThrowInAfterEach = Optional.empty(); FailureTestCase.exceptionToThrowInAfterEach = Optional.empty();
} }


private static class FailureTestCase { static class FailureTestCase {


static Optional<Throwable> exceptionToThrowInBeforeAll = Optional.empty(); static Optional<Throwable> exceptionToThrowInBeforeAll = Optional.empty();
static Optional<Throwable> exceptionToThrowInAfterAll = Optional.empty(); static Optional<Throwable> exceptionToThrowInAfterAll = Optional.empty();
Expand Down
Expand Up @@ -74,14 +74,14 @@ private void assertExecutionResults(Class<?> invalidTestClass) {


// ------------------------------------------------------------------------- // -------------------------------------------------------------------------


private static class TestCase { static class TestCase {


@Test @Test
void test() { void test() {
} }
} }


private static class TestCaseWithInvalidBeforeAllMethod { static class TestCaseWithInvalidBeforeAllMethod {


// must be static // must be static
@BeforeAll @BeforeAll
Expand All @@ -93,7 +93,7 @@ void test() {
} }
} }


private static class TestCaseWithInvalidAfterAllMethod { static class TestCaseWithInvalidAfterAllMethod {


// must be static // must be static
@AfterAll @AfterAll
Expand All @@ -105,7 +105,7 @@ void test() {
} }
} }


private static class TestCaseWithInvalidBeforeEachMethod { static class TestCaseWithInvalidBeforeEachMethod {


// must NOT be static // must NOT be static
@BeforeEach @BeforeEach
Expand All @@ -117,7 +117,7 @@ void test() {
} }
} }


private static class TestCaseWithInvalidAfterEachMethod { static class TestCaseWithInvalidAfterEachMethod {


// must NOT be static // must NOT be static
@AfterEach @AfterEach
Expand Down
Expand Up @@ -45,7 +45,7 @@ void testAndRepeatedTest(LogRecordListener listener) {
// @formatter:on // @formatter:on
} }


private static class TestCase { static class TestCase {


@Test @Test
@RepeatedTest(1) @RepeatedTest(1)
Expand Down
Expand Up @@ -95,7 +95,7 @@ void inheritedNestedTestsAreExecuted() {


// ------------------------------------------------------------------- // -------------------------------------------------------------------


private static class TestCaseWithNesting { static class TestCaseWithNesting {


@Test @Test
void someTest() { void someTest() {
Expand All @@ -115,7 +115,7 @@ void failing() {
} }
} }


private static class TestCaseWithDoubleNesting { static class TestCaseWithDoubleNesting {


static int beforeTopCount = 0; static int beforeTopCount = 0;
static int beforeNestedCount = 0; static int beforeNestedCount = 0;
Expand Down
Expand Up @@ -86,7 +86,7 @@ void executeTestCaseWithOverloadedMethodsWithSingleMethodThatAcceptsArgumentsSel
assertTrue(first.isPresent()); assertTrue(first.isPresent());
} }


private static class TestCase { static class TestCase {


@Test @Test
void test() { void test() {
Expand Down
Expand Up @@ -122,7 +122,7 @@ void testsFailWhenAfterEachFails() {
assertTrue(TestCaseWithFailingAfter.testExecuted, "test executed?"); assertTrue(TestCaseWithFailingAfter.testExecuted, "test executed?");
} }


private static class MyStandardTestCase { static class MyStandardTestCase {


static int countBefore1 = 0; static int countBefore1 = 0;
static int countBefore2 = 0; static int countBefore2 = 0;
Expand Down Expand Up @@ -165,7 +165,7 @@ void abortedTest() {


} }


private static class FirstOfTwoTestCases { static class FirstOfTwoTestCases {


@Test @Test
void succeedingTest1() { void succeedingTest1() {
Expand All @@ -184,7 +184,7 @@ void failingTest() {


} }


private static class SecondOfTwoTestCases { static class SecondOfTwoTestCases {


@Test @Test
void succeedingTest1() { void succeedingTest1() {
Expand All @@ -203,7 +203,7 @@ void succeedingTest3() {


} }


private static class TestCaseWithFailingBefore { static class TestCaseWithFailingBefore {


static int countBefore = 0; static int countBefore = 0;


Expand All @@ -223,7 +223,7 @@ void test2() {


} }


private static class TestCaseWithFailingAfter { static class TestCaseWithFailingAfter {


static boolean testExecuted = false; static boolean testExecuted = false;


Expand Down
Expand Up @@ -166,7 +166,7 @@ void superTest() {
} }
} }


private static class LocalTestCase extends AbstractTestCase { static class LocalTestCase extends AbstractTestCase {


boolean throwExceptionInAfterMethod = false; boolean throwExceptionInAfterMethod = false;


Expand Down Expand Up @@ -215,7 +215,7 @@ void alwaysFails() {
} }
} }


private static class TestCase1 { static class TestCase1 {


@BeforeAll @BeforeAll
static void beforeAll1() { static void beforeAll1() {
Expand All @@ -238,7 +238,7 @@ static void afterAll1() {
} }
} }


private static class TestCase2 extends TestCase1 { static class TestCase2 extends TestCase1 {


@BeforeAll @BeforeAll
static void beforeAll2() { static void beforeAll2() {
Expand All @@ -261,7 +261,7 @@ static void afterAll2() {
} }
} }


private static class TestCase3 extends TestCase2 { static class TestCase3 extends TestCase2 {


@BeforeAll @BeforeAll
static void beforeAll3() { static void beforeAll3() {
Expand Down
Expand Up @@ -147,7 +147,7 @@ private void performAssertions(Class<?> testClass, Map<String, String> configPar
// ------------------------------------------------------------------------- // -------------------------------------------------------------------------


@TestInstance(PER_METHOD) @TestInstance(PER_METHOD)
private static class ExplicitInstancePerTestMethodTestCase { static class ExplicitInstancePerTestMethodTestCase {


@BeforeAll @BeforeAll
static void beforeAll() { static void beforeAll() {
Expand All @@ -171,7 +171,7 @@ static void afterAllStatic() {
* {@code @AfterAll} methods are static, even though there is no explicit * {@code @AfterAll} methods are static, even though there is no explicit
* {@code @TestInstance} declaration. * {@code @TestInstance} declaration.
*/ */
private static class AssumedInstancePerTestMethodTestCase { static class AssumedInstancePerTestMethodTestCase {


@BeforeAll @BeforeAll
static void beforeAll() { static void beforeAll() {
Expand All @@ -191,7 +191,7 @@ static void afterAllStatic() {
} }


@TestInstance(PER_CLASS) @TestInstance(PER_CLASS)
private static class ExplicitInstancePerClassTestCase { static class ExplicitInstancePerClassTestCase {


@BeforeAll @BeforeAll
void beforeAll(TestInfo testInfo) { void beforeAll(TestInfo testInfo) {
Expand All @@ -215,7 +215,7 @@ void afterAll(TestInfo testInfo) {
* {@code @AfterAll} methods are non-static, even though there is no * {@code @AfterAll} methods are non-static, even though there is no
* explicit {@code @TestInstance} declaration. * explicit {@code @TestInstance} declaration.
*/ */
private static class AssumedInstancePerClassTestCase { static class AssumedInstancePerClassTestCase {


@BeforeAll @BeforeAll
void beforeAll(TestInfo testInfo) { void beforeAll(TestInfo testInfo) {
Expand Down

1 comment on commit e4789f3

@marcphilipp
Copy link
Member

Choose a reason for hiding this comment

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

👍

Please sign in to comment.