Quarkus: QuarkusTestBeforeAllCallback is run before every @Test method

[llowinge]

Describe the bug
Implementing QuarkusTestBeforeAllCallback like follows

public class CustomBeforeAllCallback implements QuarkusTestBeforeAllCallback{
    @Override
    public void beforeAll(Object testInstance) {
        System.out.println("before all message");
    }
}

Test methods like follows

    @Test
    public void testHelloEndpoint() {
        System.out.println("first");
    }

    @Test
    public void anotherTest() {
        System.out.println("second");
    }

gives me

before all message
second
before all message
first

Expected behavior
The callback is run before all @Test methods only once. So in case of example it would look like

before all message
second
first

Actual behavior
The callback is run before every @Test method.

To Reproduce

Link to a small reproducer (preferably a Maven project if the issue is not Gradle-specific).

Or attach an archive containing the reproducer to the issue.
code-with-quarkus 2.zip

Steps to reproduce the behavior:

1. Unzip reproducer
2. Run mvn clean test
3.

Configuration

# Add your application.properties here, if applicable.

Screenshots
(If applicable, add screenshots to help explain your problem.)

Environment (please complete the following information):

• Output of uname -a or ver:

Darwin MacBook-Pro 19.4.0 Darwin Kernel Version 19.4.0: Wed Mar  4 22:28:40 PST 2020; root:xnu-6153.101.6~15/RELEASE_X86_64 x86_64

• Output of java -version:

openjdk version "11.0.7" 2020-04-14
OpenJDK Runtime Environment AdoptOpenJDK (build 11.0.7+10)
OpenJDK 64-Bit Server VM AdoptOpenJDK (build 11.0.7+10, mixed mode)

• GraalVM version (if different from Java):
• Quarkus version or git rev:

 <quarkus.platform.version>1.8.1.Final</quarkus.platform.version>

• Build tool (ie. output of mvnw --version or gradlew --version):

Apache Maven 3.6.2 (40f52333136460af0dc0d7232c0dc0bcf0d9e117; 2019-08-27T17:06:16+02:00)

Additional context
(Add any other context about the problem here.)

[famod]

@geoand Just a heads up: The reproducer has a "java" suffix where it does not belong (in the services file) and the callback and the services file is located in main instead of test. But fixing all this does not fix the original problem.

Just a guess: Since a JUnit test in re-instantiated for each test method, interceptTestClassConstructor() might not be the right place to call beforeAll.

[geoand]

Thanks.

I saw that and planned accordingly locally.

Actually what we do is the proper behavior (as this callback is meant to operate on the newly constructed test class). The problem is basically the naming of the callback - it should be named AfterConstructCallback or something...
And of course JUnit's default behavior is to construct the test object for each test so the behavior we follow is correct - the naming though is terrible (of which I am guilty...)

I would love to change the name of this interface, however that would break existing integrations :(. Perhaps I should introduce a new one and deprecate the old one?
Also @llowinge perhaps a PreConstructCallback would be useful for what you want to do?

[famod]

Ah, I see.

Wouldn't a PreConstructCallback more or less behave the same as "before all"? I am just wondering what people are more familiar with.

Perhaps I should introduce a new one and deprecate the old one?

Plus logging a warning and adding a note to the migration guide, maybe?

[geoand]

Ah, I see.

Wouldn't a PreConstructCallback more or less behave the same as "before all"? I am just wondering what people are more familiar with.

Yes exactly. But since I can't call it QuarkusTestBeforeAllCallback as I stupidly named the existing interface such, we would have to settle for QuarkusTestPreConstructCallback

Perhaps I should introduce a new one and deprecate the old one?

Plus logging a warning and adding a note to the migration guide, maybe?

Yeah, I'll add a note for sure, but I don't really know what kind of warning we should have in the logs.

[famod]

Sorry, might have mixed up a few things:
A PreConstructCallback would be called exactly as often as a AfterConstructCallback, no?
If this is the case then PreConstructCallback would _not_ be (more or less) equal to "before all".

So I wonder why we don't just "fix" the invocation of BeforeAllCallback, introduce AfterConstructCallback and add a note to the migration guide?

PS: QuarkusTest prefix omitted everywhere due to laziness. ;-)

[geoand]

PreConstructCallback if we decide we need it, would be called when JUnit calls before all - which is before the test object is created.

I don't really want to repurpose the BeforeAllCallback callback because I really think it would make things more confusing for existing users of it.

[famod]

PreConstructCallback if we decide we need it, would be called when JUnit calls before all - which is before the test object is created.

Probably nitpicking here, but isn't each construction of an instance "pre construct"?

I don't really want to repurpose the BeforeAllCallback callback because I really think it would make things more confusing for existing users of it.

The irony is that the name _and_ JavaDoc of that class are consistent, just not the way it is invoked by the test extension.

[geoand]

Yeah, it was a real shame I messed up the BeforeAll callback naming...

I'm open to other suggestions on the proper before all callback name - the only thing I am against is re-purposing the old one

[famod]

Naming is hard. More often than not you spend 80% of the time on finding proper names (and 20% implementing the logic) just to have someone show up the next day saying "but have you considered ?". 😆

[geoand]

😂

[llowinge]

Hi @geoand , thanks for working on this issue, i have question: am i (or will i be) able to setup some callback (with whatever name) which will run before every @Test method ?

[geoand]

@llowinge that capability already exists - it's the QuarkusTestBeforeEachCallback

[llowinge]

@geoand Sorry, i wrote it wrong. I meant before all @Test method.

[geoand]

I'll update the PR to add a second commit that will enable this capability.

And preference for the name? See my previous discussion with @famod for details

[geoand]

Any more input on the naming of the new test callback?

[famod]

QuarkusTestPreFirstConstructCallback?
Rather "bulky" but more specific than without the "First" infix.
Another non-ideal suggestion: QuarkusTestBeforeTestClassCallback or QuarkusTestBeforeClassCallback.

[geoand]

I like the last suggestion a lot, thanks!

I'll make sure to carve out some time to get this done for 1.9

[famod]

"BeforeClass" has a retro feeling to it (Junit4) but since "BeforeAll" is burnt, I don't think it will get much better than this. 🙂