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

io.quarkus.logging.Log breaks unit tests #33210

Closed
stuartwdouglas opened this issue May 9, 2023 · 5 comments · Fixed by #33700
Closed

io.quarkus.logging.Log breaks unit tests #33210

stuartwdouglas opened this issue May 9, 2023 · 5 comments · Fixed by #33700
Assignees
Labels
Milestone

Comments

@stuartwdouglas
Copy link
Member

Describe the bug

While the automatic logger is convenient it can't be used outside of @QuarkusTest (i.e. if you just want to test some logic that does not depend on any services).

This logger should detect if it is being used in a test and fall back to inefficient logging done by walking the call stack for the case where it has not had the bytecode magic applied.

Expected behavior

No response

Actual behavior

No response

How to Reproduce?

No response

Output of uname -a or ver

No response

Output of java -version

No response

GraalVM version (if different from Java)

No response

Quarkus version or git rev

No response

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

No response

Additional information

No response

@stuartwdouglas stuartwdouglas added the kind/bug Something isn't working label May 9, 2023
@geoand
Copy link
Contributor

geoand commented May 9, 2023

cc @Ladicek

@Ladicek
Copy link
Contributor

Ladicek commented May 9, 2023

It would be fairly easy to allow io.quarkus.logging.Log usage even without bytecode transformation, but as you say, that's inefficient. So I take it your proposal is to only allow this for testing? Since you're specifically talking about non-@QuarkusTest tests, I don't think there's a good way to recognize if the current execution is a test or not. Any ideas?

(The only thing I can think of is looking for org.junit and possibly org.testng methods on the call stack. This is also expensive, so we could do it once, during class initialization, and store the result it in a static final field. This assumes that if the Log class is used in a test once, it will always be used for tests in that classloader. It also assumes that in the testing scenario, noone will use the Log class before the test framework kicks in. Both seem to be fairly safe assumptions, but it's still a bit more fragile than I'd like.)

@Ladicek
Copy link
Contributor

Ladicek commented May 9, 2023

Another option is: just stop failing, period. The static methods on the io.quarkus.logging.Log class are never called at runtime in Quarkus (because the bytecode transformation changes all call sites to call instance methods on automatically-generated Logger instances), so we don't necessarily have to care about them being inefficient. We could just implement them properly (which would entail stack walking).

The issue with this is that libraries that don't go through the Quarkus bytecode transformation would suddenly be able to use this API (they can't today), which would come with significant overhead, and it's not exactly obvious when the API has overhead and when it doesn't.

So I'd prefer to not do this unconditionally. Configuration is probably a bad idea. Detecting test environment seems like a best choice to me (possibly with an undocumented system property to override), except the only way I can think of (see previous comment) is dubious.

@stuartwdouglas
Copy link
Member Author

I would look for JUnit on the class path, and if it is present allow static usage.

@mkouba
Copy link
Contributor

mkouba commented May 29, 2023

I think that io.quarkus.runtime.LaunchMode.current() == TEST could cover most of the cases because of io.quarkus.test.junit.BasicLoggingEnabler, i.e. it should work if quarkus-junit5 is on the class path.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants