Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change default behavior of @WithTestResource #42852

Merged
merged 2 commits into from
Sep 4, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
package io.quarkus.test.common;

/**
* Defines how Quarkus behaves with regard to the application of the resource to this test and the testsuite in general
*/
public enum TestResourceScope {

/*
* The declaration order must be from the narrowest scope to the widest
*/

/**
* Means that Quarkus will run the test in complete isolation, i.e. it will restart every time it finds such a resource
*/
RESTRICTED_TO_CLASS,
/**
* Means that Quarkus will not restart when running consecutive tests that use the same resource
Copy link
Member

Choose a reason for hiding this comment

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

The same resource or the same set of resources? If the former how do you group to restart the resources less often?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Users can't really influence the order of test execution, but if Quarkus does its job correctly, restarting will be minimized

Copy link
Member

Choose a reason for hiding this comment

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

How? (for my benefit).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

@emmanuelbernard emmanuelbernard Sep 3, 2024

Choose a reason for hiding this comment

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

Also my question was
@WithTestResource(value={Resource1.class, Resource2.class, Resource3.class})
will it run in the same batch as @WithTestResource(value={Resource3.class})
what about @WithTestResource(value={Resource3.class, Resource4.class})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Even if those are executed one after the other, they still force a restart because they don't match

Copy link
Member

Choose a reason for hiding this comment

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

ok, thanks. So i'ts not the same resource that matches, it's the exact resource set (or list of resoruces) that matches.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct!

*/
MATCHING_RESOURCE,

/**
* Means the resource applies to all tests in the testsuite
*/
GLOBAL
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,20 @@
/**
* Used to define a test resource, which can affect various aspects of the application lifecycle.
* <p>
* As of Quarkus 3.16, the default behavior of the annotation (meaning that {@code scope} has not been set)
* is that test classes annotated with the same {@code WithTestResource} will <strong>not</strong> force a restart
* of Quarkus.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe I'm blind but while I see the code to order the restricted to class first, I'm not exactly sure how the tests with the same resources are actually grouped?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you talking about QuarkusTestProfileAwareClassOrderer? I haven't changed that

Copy link
Member

Choose a reason for hiding this comment

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

Yes I'm talking about this specific class. I think someone said it was ordering classes depending on resources and I don't see that in the code. I just see some grouping by if they have per-class test resources.

Don't we want to group classes which have similar test resources?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can look into this in a follow up (or maybe @famod can since he wrote the ordering code).
For the time being, I would like to get this if we agree it's the proper functionality as the change has a very high chance of conflicting with any changes made in test handling code.

Copy link
Member

Choose a reason for hiding this comment

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

I don't see that in the code

At a first glimpse I don't see that either. 🤔 It is has been a while, so I'd need some time to look into it.
Until when do we need that adjustment (if needed after all)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is has been a while, so I'd need some time to look into it. Until when do we need that adjustment (if needed after all)?

As this PR is breaking and meant to go into 3.16, we still have a weeks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Btw, @QuarkusComponentTest isn't covered either but I haven't looked into whether grouping such tests would make sense at all.

Yeah, it probably makes sense to have those executed at the beginning or the end of the run, but that's definitely something for after this PR is in.

Copy link
Member

Choose a reason for hiding this comment

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

Btw, @QuarkusComponentTest isn't covered either but I haven't looked into whether grouping such tests would make sense at all.

Given they are supposed to be faster and lower level, let's execute them first.
But yes, definitely out of the scope for this PR.

Copy link
Member

Choose a reason for hiding this comment

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

FYI, I haven't had the time to take a stab at the QuarkusTestProfileAwareClassOrderer change. I might have some time for that by the end of next week.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🙏🏽

* <p>
* The equivalent behavior to {@code QuarkusTestResource(restrictToAnnotatedClass = false)} is to use
* {@code WithTestResource(scope = TestResourceScope.GLOBAL)},
* while the equivalent behavior to {@code QuarkusTestResource(restrictToAnnotatedClass = true)} is to use
* {@code WithTestResource(scope = TestResourceScope.RESTRICTED_TO_CLASS)},
* <p>
* WARNING: this annotation, introduced in 3.13, caused some issues so it was decided to undeprecate
* {@link QuarkusTestResource} and rework the behavior of this annotation. For now, we recommend not using it
* until we improve its behavior.
* <p>
* Note: When using the {@code restrictToAnnotatedClass=true} (which is the default), each test that is annotated
* Note: When using the {@code scope=TestResourceScope.RESTRICTED_TO_CLASS}, each test that is annotated
* with {@code @WithTestResource} will result in the application being re-augmented and restarted (in a similar fashion
* as happens in dev-mode when a change is detected) in order to incorporate the settings configured by the annotation.
* If there are many instances of the annotation used throughout the testsuite, this could result in slow test execution.
Expand All @@ -28,14 +37,6 @@
* started <b>before</b> <b>any</b> test is run.
* <p>
* Note that test resources are never restarted when running {@code @Nested} test classes.
* <p>
* The only difference with {@link QuarkusTestResource} is that the default value for
* {@link #restrictToAnnotatedClass()} {@code == true}.
* </p>
* <p>
* This means that any resources managed by {@link #value()} apply to an individual test class or test profile, unlike with
* {@link QuarkusTestResource} where a resource applies to all test classes.
* </p>
*/
@Target(ElementType.TYPE)
@Retention(RetentionPolicy.RUNTIME)
Expand All @@ -61,15 +62,11 @@
boolean parallel() default false;

/**
* Whether this annotation should only be enabled if it is placed on the currently running test class or test profile.
* Note that this defaults to true for meta-annotations since meta-annotations are only considered
* for the current test class or test profile.
* <p>
* Note: When this is set to {@code true} (which is the default), the annotation {@code @WithTestResource} will result
* in the application being re-augmented and restarted (in a similar fashion as happens in dev-mode when a change is
* detected) in order to incorporate the settings configured by the annotation.
* Defines how Quarkus behaves with regard to the application of the resource to this test and the test-suite in general.
* The default is {@link TestResourceScope#MATCHING_RESOURCE} which means that if two tests are annotated with the same
* {@link WithTestResource} annotation, no restart will take place between tests.
*/
boolean restrictToAnnotatedClass() default true;
TestResourceScope scope() default TestResourceScope.MATCHING_RESOURCE;

@Target(ElementType.TYPE)
@Retention(RetentionPolicy.RUNTIME)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ void testTestInjector(Class<?> clazz) {
Assertions.assertEquals("dummy", foo.dummy.value);
}

@WithTestResource(value = UsingTestInjectorLifecycleManager.class, restrictToAnnotatedClass = false)
@WithTestResource(value = UsingTestInjectorLifecycleManager.class, scope = TestResourceScope.GLOBAL)
public static class UsingInjectorTest {
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
package io.quarkus.test.common;

import static io.quarkus.test.common.TestResourceManager.testResourcesRequireReload;
import static io.quarkus.test.common.TestResourceScope.*;
import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;

import java.util.Collections;
import java.util.Set;

import org.junit.jupiter.api.Test;

import io.quarkus.test.common.TestResourceManager.TestResourceComparisonInfo;

public class TestResourceManagerReloadTest {

@Test
public void emptyResources() {
assertFalse(testResourcesRequireReload(Collections.emptySet(), Set.of()));
}

@Test
public void differentCount() {
assertTrue(testResourcesRequireReload(Collections.emptySet(),
Set.of(new TestResourceComparisonInfo("test", RESTRICTED_TO_CLASS))));

assertTrue(testResourcesRequireReload(Set.of(new TestResourceComparisonInfo("test", RESTRICTED_TO_CLASS)),
Collections.emptySet()));
}

@Test
public void sameSingleRestrictedToClassResource() {
assertTrue(testResourcesRequireReload(
Set.of(new TestResourceComparisonInfo("test", RESTRICTED_TO_CLASS)),
Set.of(new TestResourceComparisonInfo("test", RESTRICTED_TO_CLASS))));
}

@Test
public void sameSingleMatchingResource() {
assertFalse(testResourcesRequireReload(
Set.of(new TestResourceComparisonInfo("test", MATCHING_RESOURCE)),
Set.of(new TestResourceComparisonInfo("test", MATCHING_RESOURCE))));
}

@Test
public void differentSingleMatchingResource() {
assertTrue(testResourcesRequireReload(
Set.of(new TestResourceComparisonInfo("test", MATCHING_RESOURCE)),
Set.of(new TestResourceComparisonInfo("test2", MATCHING_RESOURCE))));
}

@Test
public void sameMultipleMatchingResource() {
assertFalse(testResourcesRequireReload(
Set.of(
new TestResourceComparisonInfo("test", MATCHING_RESOURCE),
new TestResourceComparisonInfo("test2", MATCHING_RESOURCE),
new TestResourceComparisonInfo("test3", GLOBAL)),
Set.of(new TestResourceComparisonInfo("test3", GLOBAL),
new TestResourceComparisonInfo("test2", MATCHING_RESOURCE),
new TestResourceComparisonInfo("test", MATCHING_RESOURCE))));
}

@Test
public void differentMultipleMatchingResource() {
assertTrue(testResourcesRequireReload(
Set.of(
new TestResourceComparisonInfo("test", MATCHING_RESOURCE),
new TestResourceComparisonInfo("test2", MATCHING_RESOURCE),
new TestResourceComparisonInfo("test3", GLOBAL)),
Set.of(new TestResourceComparisonInfo("test3", GLOBAL),
new TestResourceComparisonInfo("test2", MATCHING_RESOURCE),
new TestResourceComparisonInfo("TEST", MATCHING_RESOURCE))));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,8 @@ void testParallelResourcesRunInParallel(Class<?> clazz) {
Assertions.assertEquals("value2", props.get("key2"));
}

@WithTestResource(value = FirstLifecycleManager.class, restrictToAnnotatedClass = false)
@WithTestResource(value = SecondLifecycleManager.class, restrictToAnnotatedClass = false)
@WithTestResource(value = FirstLifecycleManager.class, scope = TestResourceScope.GLOBAL)
@WithTestResource(value = SecondLifecycleManager.class, scope = TestResourceScope.GLOBAL)
public static class MyTest {
}

Expand Down Expand Up @@ -99,8 +99,8 @@ public void stop() {
}
}

@WithTestResource(value = FirstSequentialQuarkusTestResource.class, restrictToAnnotatedClass = false)
@WithTestResource(value = SecondSequentialQuarkusTestResource.class, restrictToAnnotatedClass = false)
@WithTestResource(value = FirstSequentialQuarkusTestResource.class, scope = TestResourceScope.GLOBAL)
@WithTestResource(value = SecondSequentialQuarkusTestResource.class, scope = TestResourceScope.GLOBAL)
public static class SequentialTestResourcesTest {
}

Expand Down Expand Up @@ -150,8 +150,8 @@ public int order() {
}
}

@WithTestResource(value = FirstParallelQuarkusTestResource.class, parallel = true, restrictToAnnotatedClass = false)
@WithTestResource(value = SecondParallelQuarkusTestResource.class, parallel = true, restrictToAnnotatedClass = false)
@WithTestResource(value = FirstParallelQuarkusTestResource.class, parallel = true, scope = TestResourceScope.GLOBAL)
@WithTestResource(value = SecondParallelQuarkusTestResource.class, parallel = true, scope = TestResourceScope.GLOBAL)
public static class ParallelTestResourcesTest {
}

Expand Down Expand Up @@ -257,7 +257,7 @@ public void stop() {
}
}

@WithTestResource(value = AnnotationBasedQuarkusTestResource.class, restrictToAnnotatedClass = false)
@WithTestResource(value = AnnotationBasedQuarkusTestResource.class, scope = TestResourceScope.GLOBAL)
@Retention(RetentionPolicy.RUNTIME)
@Target(ElementType.TYPE)
@Repeatable(WithAnnotationBasedTestResource.List.class)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
import static io.quarkus.test.common.PathTestHelper.getTestClassesLocation;

import java.io.IOException;
import java.lang.annotation.Annotation;
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.Paths;
Expand Down Expand Up @@ -39,10 +38,8 @@
import io.quarkus.paths.PathList;
import io.quarkus.runtime.LaunchMode;
import io.quarkus.test.common.PathTestHelper;
import io.quarkus.test.common.QuarkusTestResource;
import io.quarkus.test.common.RestorableSystemProperties;
import io.quarkus.test.common.TestClassIndexer;
import io.quarkus.test.common.WithTestResource;

public class AbstractJvmQuarkusTestExtension extends AbstractQuarkusTestWithContextExtension {

Expand Down Expand Up @@ -270,43 +267,6 @@ private Class<? extends QuarkusTestProfile> findTestProfileAnnotation(Class<?> c
return null;
}

protected static boolean hasPerTestResources(ExtensionContext extensionContext) {
return hasPerTestResources(extensionContext.getRequiredTestClass());
}

public static boolean hasPerTestResources(Class<?> requiredTestClass) {
while (requiredTestClass != Object.class) {
for (WithTestResource testResource : requiredTestClass.getAnnotationsByType(WithTestResource.class)) {
if (testResource.restrictToAnnotatedClass()) {
return true;
}
}

for (QuarkusTestResource testResource : requiredTestClass.getAnnotationsByType(QuarkusTestResource.class)) {
if (testResource.restrictToAnnotatedClass()) {
return true;
}
}
// scan for meta-annotations
for (Annotation annotation : requiredTestClass.getAnnotations()) {
// skip TestResource annotations
var annotationType = annotation.annotationType();

if ((annotationType != WithTestResource.class) && (annotationType != QuarkusTestResource.class)) {
// look for a TestResource on the annotation itself
if ((annotationType.getAnnotationsByType(WithTestResource.class).length > 0)
|| (annotationType.getAnnotationsByType(QuarkusTestResource.class).length > 0)) {
// meta-annotations are per-test scoped for now
return true;
}
}
}
// look up
requiredTestClass = requiredTestClass.getSuperclass();
}
return false;
}

protected static class PrepareResult {
protected final AugmentAction augmentAction;
protected final QuarkusTestProfile profileInstance;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,6 @@
import java.io.FileInputStream;
import java.io.IOException;
import java.io.UncheckedIOException;
import java.lang.annotation.Annotation;
import java.lang.reflect.Constructor;
import java.lang.reflect.Field;
import java.lang.reflect.InvocationTargetException;
import java.net.URI;
Expand All @@ -19,10 +17,7 @@
import java.nio.file.Path;
import java.nio.file.Paths;
import java.security.CodeSource;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Optional;
import java.util.Properties;
Expand Down Expand Up @@ -155,38 +150,6 @@ static TestProfileAndProperties determineTestProfileAndProperties(Class<? extend
return new TestProfileAndProperties(testProfile, properties);
}

/**
* Since {@link TestResourceManager} is loaded from the ClassLoader passed in as an argument,
* we need to convert the user input {@link QuarkusTestProfile.TestResourceEntry} into instances of
* {@link TestResourceManager.TestResourceClassEntry}
* that are loaded from that ClassLoader
*/
static <T> List<T> getAdditionalTestResources(
QuarkusTestProfile profileInstance, ClassLoader classLoader) {
if ((profileInstance == null) || profileInstance.testResources().isEmpty()) {
return Collections.emptyList();
}

try {
Constructor<?> testResourceClassEntryConstructor = Class
.forName(TestResourceManager.TestResourceClassEntry.class.getName(), true, classLoader)
.getConstructor(Class.class, Map.class, Annotation.class, boolean.class);

List<QuarkusTestProfile.TestResourceEntry> testResources = profileInstance.testResources();
List<T> result = new ArrayList<>(testResources.size());
for (QuarkusTestProfile.TestResourceEntry testResource : testResources) {
T instance = (T) testResourceClassEntryConstructor.newInstance(
Class.forName(testResource.getClazz().getName(), true, classLoader), testResource.getArgs(),
null, testResource.isParallel());
result.add(instance);
}

return result;
} catch (Exception e) {
throw new IllegalStateException("Unable to handle profile " + profileInstance.getClass(), e);
}
}

static void startLauncher(ArtifactLauncher launcher, Map<String, String> additionalProperties, Runnable sslSetter)
throws IOException {
launcher.includeAsSysProps(additionalProperties);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,13 @@
import static io.quarkus.test.junit.IntegrationTestUtil.doProcessTestInstance;
import static io.quarkus.test.junit.IntegrationTestUtil.ensureNoInjectAnnotationIsUsed;
import static io.quarkus.test.junit.IntegrationTestUtil.findProfile;
import static io.quarkus.test.junit.IntegrationTestUtil.getAdditionalTestResources;
import static io.quarkus.test.junit.IntegrationTestUtil.getArtifactType;
import static io.quarkus.test.junit.IntegrationTestUtil.getSysPropsToRestore;
import static io.quarkus.test.junit.IntegrationTestUtil.handleDevServices;
import static io.quarkus.test.junit.IntegrationTestUtil.readQuarkusArtifactProperties;
import static io.quarkus.test.junit.IntegrationTestUtil.startLauncher;
import static io.quarkus.test.junit.TestResourceUtil.testResourcesRequireReload;
import static io.quarkus.test.junit.TestResourceUtil.TestResourceManagerReflections.copyEntriesFromProfile;

import java.io.Closeable;
import java.io.File;
Expand Down Expand Up @@ -75,7 +76,6 @@ public class QuarkusIntegrationTestExtension extends AbstractQuarkusTestWithCont
private static Throwable firstException; //if this is set then it will be thrown from the very first test that is run, the rest are aborted

private static Class<?> currentJUnitTestClass;
private static boolean hasPerTestResources;

private static Map<String, String> devServicesProps;
private static String containerNetworkId;
Expand Down Expand Up @@ -154,9 +154,9 @@ private QuarkusTestExtensionState ensureStarted(ExtensionContext extensionContex
currentJUnitTestClass = extensionContext.getRequiredTestClass();
}
// we reload the test resources if we changed test class and if we had or will have per-test test resources
boolean reloadTestResources = isNewTestClass
&& (hasPerTestResources || QuarkusTestExtension.hasPerTestResources(extensionContext));
if ((state == null && !failedBoot) || wrongProfile || reloadTestResources) {
boolean reloadTestResources = false;
if ((state == null && !failedBoot) || wrongProfile || (reloadTestResources = isNewTestClass
geoand marked this conversation as resolved.
Show resolved Hide resolved
&& TestResourceUtil.testResourcesRequireReload(state, extensionContext.getRequiredTestClass()))) {
if (wrongProfile || reloadTestResources) {
if (state != null) {
try {
Expand Down Expand Up @@ -217,15 +217,15 @@ private QuarkusTestExtensionState doProcessStart(Properties quarkusArtifactPrope
TestProfileAndProperties testProfileAndProperties = determineTestProfileAndProperties(profile, sysPropRestore);

testResourceManager = new TestResourceManager(requiredTestClass, quarkusTestProfile,
getAdditionalTestResources(testProfileAndProperties.testProfile,
copyEntriesFromProfile(testProfileAndProperties.testProfile,
context.getRequiredTestClass().getClassLoader()),
testProfileAndProperties.testProfile != null
&& testProfileAndProperties.testProfile.disableGlobalTestResources(),
devServicesProps, containerNetworkId == null ? Optional.empty() : Optional.of(containerNetworkId));
testResourceManager.init(
testProfileAndProperties.testProfile != null ? testProfileAndProperties.testProfile.getClass().getName()
: null);
hasPerTestResources = testResourceManager.hasPerTestResources();

if (isCallbacksEnabledForIntegrationTests()) {
populateCallbacks(requiredTestClass.getClassLoader());
}
Expand Down
Loading
Loading