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

Logging configuration leaks from one test to the next #43180

Open
yrodiere opened this issue Sep 10, 2024 · 9 comments
Open

Logging configuration leaks from one test to the next #43180

yrodiere opened this issue Sep 10, 2024 · 9 comments

Comments

@yrodiere
Copy link
Member

Describe the bug

While working on #43074 I noticed that when I set system properties related to logging (e.g. quarkus.logging."org.hibernate".level=TRACE) before a test using QuarkusUnitTest, then these properties still apply in test classes that execute afterwards -- even if I reset them after each test!

I thought "fair enough, setting system properties dynamically probably isn't recommended anyway".

But then I started doing something like this:

    @RegisterExtension
    static QuarkusUnitTest runner = new QuarkusUnitTest()
            .withApplicationRoot((jar) -> jar
                    .addClass(MyMappedSuperclass.class)
                    .addClass(MyAbstractEntity.class)
                    .addClass(MyConcreteEntity.class)
                    .addClass(FieldAccessEnhancedDelegate.class))
            .withConfigurationResource("application.properties")
            .overrideConfigKey("quarkus.hibernate-orm.log.bind-parameters", "true");

... which simply sets a Quarkus property related to logging (bind-parameters) in application.properties.
And again, that setting affected test classes that executed afterwards.

So, it seems to me QuarkusUnitTest leaks logging configuration from one test to the next... somehow.

QuarkusUnitTest being internal, it's not that bad, but what worries me more is the possibility that this leak could also happen in actual application tests, for example those that use a QuarkusTestProfile to enable tracing in one particular test only. I did not try that, but that's something we should investigate to decide how critical this leak is.

Expected behavior

Customizing log configuration in one test should not affect following test classes in the same run.

Actual behavior

Customizing log configuration in one test affects all following test classes in the same run.

How to Reproduce?

  1. Clone https://github.com/yrodiere/quarkus/tree/test-logging-config-leak (note it's branch test-logging-config-leak, not main)
  2. Notice in the Git history that the logging config is modified in PublicFieldAccessInheritanceTest through property quarkus.hibernate-orm.log.bind-parameters.
  3. Notice in the Git history that the logging config is not modified in LogBindParametersDefaultValueTest.
  4. Build Quarkus locally.
  5. Run ./mvnw clean test -pl extensions/hibernate-orm/deployment -Dtest='PublicFieldAccessInheritanceTest,LogBindParametersDefaultValueTest'
  6. Notice the test output for LogBindParametersDefaultValueTest shows TRACE logs, even though we don't enable them in this test.
  7. Notice LogBindParametersDefaultValueTest failed because of these unexpected logs.

Output of uname -a or ver

No response

Output of java -version

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

@quarkus-bot
Copy link

quarkus-bot bot commented Sep 10, 2024

/cc @radcortez (config)

@yrodiere yrodiere changed the title Logging configuration leaks from one test to the next, even when using different configuration Logging configuration leaks from one test to the next Sep 10, 2024
@radcortez
Copy link
Member

Yes, this is probably related to #27996.

Unfortunately, we need to get a few things out of the way to fix this: #42986

@mkouba
Copy link
Contributor

mkouba commented Sep 11, 2024

@yrodiere Does it only apply to logging configuration or any config property in general?

@yrodiere
Copy link
Member Author

@yrodiere Does it only apply to logging configuration or any config property in general?

It applies to anything having an effect on logging, but only logging, from what I can see.

So quarkus.logging.*, but also quarkus.hibernate-orm.log.bind-parameters which actually creates a build item that affects logging behavior.

@mkouba
Copy link
Contributor

mkouba commented Sep 12, 2024

@yrodiere Does it only apply to logging configuration or any config property in general?

It applies to anything having an effect on logging, but only logging, from what I can see.

So quarkus.logging.*, but also quarkus.hibernate-orm.log.bind-parameters which actually creates a build item that affects logging behavior.

I think that in Quarkus the logging config/context is stored in a static variable and so for example if you change the category level per test you would need to reset the level to the previous value after the test. And this "reset" action could be pretty complex. Unless there's a way to capture the "snapshot" of the logging config and apply this snapshot afterwards 🤷.

CC @dmlloyd

@yrodiere
Copy link
Member Author

yrodiere commented Sep 12, 2024

I think that in Quarkus the logging config/context is stored in a static variable and so for example if you change the category level per test you would need to reset the level to the previous value after the test.

FWIW I worked around the issue for my particular use case by working directly with Logger#setLevel; see uses of setLevel in https://github.com/quarkusio/quarkus/pull/43077/files .

Of course this doesn't change anything for application developpers setting logging-related configuration properties on a per-test basis 🤷

And this "reset" action could be pretty complex. Unless there's a way to capture the "snapshot" of the logging config and apply this snapshot afterwards 🤷.

You could alternatively just reset all levels to null after an application shutdown in test mode, so that you can reapply the whole logging configuration from a clean slate on each application startup.

@yrodiere
Copy link
Member Author

You could alternatively just reset all levels to null after an application shutdown in test mode

This makes me wonder whether dev mode is affected 🤔

@mkouba
Copy link
Contributor

mkouba commented Sep 12, 2024

FWIW I worked around the issue for my particular use case by working directly with Logger#setLevel; see uses of setLevel in https://github.com/quarkusio/quarkus/pull/43077/files .

This looks pretty useful 👍

@dmlloyd
Copy link
Member

dmlloyd commented Sep 12, 2024

You could alternatively just reset all levels to null after an application shutdown in test mode, so that you can reapply the whole logging configuration from a clean slate on each application startup.

There is more to logging than levels. Resetting all of logging is theoretically possible but probably pretty difficult. It might be easier to do/undo logging changes per test, for now.

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

No branches or pull requests

4 participants