-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Introduce QuarkusTestProfileAwareClassOrderer for efficient @TestProfile
ordering
#20156
Conversation
...and of course I will add something to the testing docs. |
public void orderClasses(ClassOrdererContext context) { | ||
AtomicInteger testProfileCounter = new AtomicInteger(); | ||
Map<Class<? extends QuarkusTestProfile>, Integer> testProfileNumberMap = new HashMap<>(); | ||
Function<Class<? extends QuarkusTestProfile>, Integer> testProfileNumberProvider = desc -> testProfileNumberMap |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really like the counter approach, as I would prefer something stable between runs.
Ideally this would match the continuous testing sort order (ideally with a shared implementation):
quarkus/core/deployment/src/main/java/io/quarkus/deployment/dev/testing/JunitTestRunner.java
Line 584 in 9950848
List<Class<?>> itClasses = new ArrayList<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's stable between runs because e.g. surefire will not change the order randomly unless you tell it to do so.
But I will have a look at what you suggested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it is stable then I think the approach is probably fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I aligned it a bit more with what you suggested (profile FQCN infix), but I don't really see much value in trying to unify it more since the constraints are different: The orderer has a single list with all tests, the runner in continuous testing has two separate ones. I also wanted to give users the choice to reconfigure the order (or even subclass it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Btw, I quickly tried this with continuous testing and users might be surprised to see a different ordering there.
But I think we can always follow up with further unifications later.
FWIW in my current project (with Quarkus 2.2.3 and overridden JUnit) we are now using a slightly simpler version of this orderer and it's working: In one module we save over 20s! |
@geoand I'm almost done revamping this, but let's say I won't make it before Sunday: I suppose this would then be still in time for 2.3.0.Final? |
To have this in for |
@TestProfile
ordering
|
||
protected static final String DEFAULT_ORDER_PREFIX_QUARKUS_TEST = "20_"; | ||
protected static final String DEFAULT_ORDER_PREFIX_QUARKUS_TEST_WITH_PROFILE = "40_"; | ||
protected static final String DEFAULT_ORDER_PREFIX_NON_QUARKUS_TEST = "60_"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: The "number spacing" might seem odd, but I wanted to leave some gaps for users to fill in their own orderings.
* Limitations: | ||
* <ul> | ||
* <li>This orderer does not (yet) consider {@linkplain io.quarkus.test.common.QuarkusTestResource#restrictToAnnotatedClass() | ||
* test resources that are restricted to the annotated class}.</li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could be a future improvement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FTR: #20420
Failing Jobs - Building a6883fe
Full information is available in the Build summary check run. Failures⚙️ MicroProfile TCKs Tests #- Failing: tcks/resteasy-reactive
📦 tcks/resteasy-reactive✖ 📦 tcks/resteasy-reactive/target/testsuite/tests✖
|
TCKs Tests cannot be related. @geoand do you want to have a look? I'd like to merge this soon so that it ends up in 2.3.0.Final. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
quarkusio/resteasy-reactive-testsuite@a43f921 should fix the intermittent failure. |
@stuartwdouglas cool, have you updated the ref? See https://github.com/quarkusio/quarkus/blob/main/tcks/resteasy-reactive/README.md?plain=1#L8 |
#20437 is taking care of using the latest ref of resteasy-reactive-testsuite |
Update: Finalized, resolves #20117
@geoand & @stuartwdouglas this is a very early draft for #20117.
I haven't tested it yet but I wanted to get some early feedback:
util
or somewhere else?W.r.t. extensibility, one use-case I have in mind for my current project is to also group/sort ArchUnit tests.
Others might want to execute simple unit tests before QuarkusTests (which they could also do by swapping the values of those two junit properties).
Btw, I will add some javadoc and inline comments as well. In case it's hard to see right now: The first
QuarkusTestProfile
class that is found receives the order number 1, which is added to the base number (1000). The next differentQuarkusTestProfile
class receives 2 and so on.PS: I'll probably have to check for
@QuarkusIntegrationTest
as well.TODOs:
QuarkusIntegrationTest