QuarkusUnitTest#overrideConfigKey() is ignored if the test archive already contains an "application.properties" asset

[mkouba]

Describe the bug

There seems to be a bug in the ShrinkWrap project - if you use both archive.addAsResource(new StringAsset("quarkus.property=false"),"application.properties") and testConfig.overrideConfigKey("quarkus.property", "true") then the value from the application.properties asset is taken.

Expected behavior

I think that at the very least we should:

1. Clarify the javadoc of QuarkusUnitTest#overrideConfigKey(), and,
2. Either fail the test or log a warning if both addResource() and overrideConfigKey() are used.

Ideally, we would merge the existing application.properties asset and the overriden config properties but I'm not sure how complicated that could be.

Actual behavior

The overriden config properties are ignored, no warning is logged.

How to Reproduce?

No response

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]

/cc @radcortez (config)

[geoand]

Either fail the test or log a warning if both addResource() and overwriteConfigProperty() are used.

Maybe we initially use a warning and then soon make it an error?

[mkouba]

Either fail the test or log a warning if both addResource() and overwriteConfigProperty() are used.

Maybe we initially use a warning and then soon make it an error?

Actually, something like this seems to work:
mkouba@1636cf4

So we could merge the existing and overriden config properties.

The problem is that this fix could actually break existing tests 🤷.

[geoand]

Yeah, it's probably better to move in a more safe direction, not a more lenient one.

[mkouba]

Yeah, it's probably better to move in a more safe direction, not a more lenient one.

Hm, I wouldn't call it "a more lenient one" but a "a convenient one".

The current behavior does not make a lot of sense because if an addResource() is used then overrideConfigKey() is ignored (due to the ShrinkWrap bug). So no "override" really. If we log a warning we should also clarify the javadoc. But we'll have to say "override" does not mean "override" because of a bug and the behavior may change in the future -> complete mess. If we decide to fail the test then existing tests could fail and the behavior still makes no sense.

So I think that we should:

1. Merge the existing application.properties and the overriden properties
2. Clarify the javadoc that overrideConfigKey() is always preferred
3. Log a message that properties were merged + add an option to show the merged properties easily

[yrodiere]

So I think that we should:

1. Merge the existing `application.properties` and the overriden properties

2. Clarify the javadoc that `overrideConfigKey()` is always preferred

3. Log a message that properties were merged + add an option to show the merged properties easily

Makes sense to me, with one doubt though: if you contribute application.properties through both addAsResource and withConfigurationResource, then IMO merging these two files is dodgy. In case of conflict on one property key, which one would win? Are there even cases when we do that and it's not just a mistake? Wouldn't we want to completely ignore the one added though addAsResource?

To be clear:

• .withConfigurationResource("...") + overrideConfigKey => Fine (already works)
• .addAsResource("...", "application.properties") + overrideConfigKey => Fine (your patch would fix it)
• .addAsResource("...", "application.properties") + .withConfigurationResource("...") (with or without overrideConfigKey) => Behavior is unclear, arguably dangerous

Also, I believe .withConfigurationResource("...") was initially added precisely because .addAsResource("...", "application.properties")didn't work withoverrideConfigKey, so... if you make it work, maybe .withConfigurationResource("...")` should be deprecated for removal?

[mkouba]

Makes sense to me, with one doubt though: if you contribute application.properties through both addAsResource and withConfigurationResource, then IMO merging these two files is dodgy. In case of conflict on one property key, which one would win? Are there even cases when we do that and it's not just a mistake? Wouldn't we want to completely ignore the one added though addAsResource?

That's a good point. I've never used withConfigurationResource() before so I forgot about this use case.

BTW withConfigurationResource() is backed by the same Properties object as overrideConfigKey(). So if you do overrideConfigKey("foo","bar") and then call withConfigurationResource() and the file contains foo=baz then the final value will be foo=baz. And if you call it the other way around the final value will be foo=bar.

.withConfigurationResource("...") + overrideConfigKey => Fine (already works)

Works but not ideally - see above.

.addAsResource("...", "application.properties") + .withConfigurationResource("...") (with or without overrideConfigKey)

Currently, the withConfigurationResource("...") properties would take precedence.

Also, I believe .withConfigurationResource("...") was initially added precisely because .addAsResource("...", "application.properties")didn't work withoverrideConfigKey, so... if you make it work, maybe .withConfigurationResource("...")` should be deprecated for removal?

Good point again ;-). Because if you need to externalize the configuration in a file then something like addAsResource("my-application.properties") and my-application.properties located in src/test/resources work just fine.

Hm, and it seems that QuarkusProdModeTest is also affected.

[radcortez]

Reading the method names, I would expect the following behaviours:

• overrideConfigKey - max ordinal in the configuration key, overriding all available sources
• withConfigurationResource - above all the classpath resources, but not system properties or env.
• addAsResource - above all the classpath resources, but not system properties, env, and withConfigurationResource

You can combine all the APIs, and the ordinality should be clear. Yes, current tests might likely break, and that is a problem.

We have inconsistencies in some places, that we should fix. Another one is the ordinality of io.quarkus.test.junit.QuarkusTestProfile#getConfigOverrides, which I would expect to have max ordinality, but it is being written as system properties. I am bringing this up so we can have a broader discussion. We may disagree on how we should order, but I think that at the very least, overrideConfigKey and io.quarkus.test.junit.QuarkusTestProfile#getConfigOverrides should be at the same ordinality.

[mkouba]

@radcortez This issue is about QuarkusUnitTest (from quarkus-junit5-internal) which IMO does not reflect the QuarkusTestProfile at all.

Reading the method names, I would expect the following behaviours:

Currently, all of these config properties are backed by the same ConfigSource - the one used to load application.properties. Which is probably not a problem because test authors control the deployment structure.

* `overrideConfigKey ` - max ordinal in the configuration key, overriding all available sources

* `withConfigurationResource` - above all the classpath resources, but not system properties or env.

* `addAsResource` - above all the classpath resources, but not system properties, env, and `withConfigurationResource`

You can combine all the APIs, and the ordinality should be clear. Yes, current tests might likely break, and that is a problem.

We have inconsistencies in some places, that we should fix. Another one is the ordinality of io.quarkus.test.junit.QuarkusTestProfile#getConfigOverrides, which I would expect to have max ordinality, but it is being written as system properties. I am bringing this up so we can have a broader discussion. We may disagree on how we should order, but I think that at the very least, overrideConfigKey and ``io.quarkus.test.junit.QuarkusUnitTest#getConfigOverrides` should be at the same ordinality.

[radcortez]

@radcortez This issue is about QuarkusUnitTest (from quarkus-junit5-internal) which IMO does not reflect the QuarkusTestProfile at all.

I know. Sorry for bringing this up here, I understand it may be confusing. I brought this up because recently, I've also encountered issues with io. quarkus.test.junit.QuarkusUnitTest#getConfigOverrides. They have very similar names, and in my opinion, they should behave in the same way, no matter which testing context you are in.

Currently, all of these config properties are backed by the same ConfigSource - the one used to load application.properties. Which is probably not a problem because test authors control the deployment structure.

Yes, I know. We could split each API into its own source with its own ordinality, making it clear who overrides who.

[mkouba]

@radcortez This issue is about QuarkusUnitTest (from quarkus-junit5-internal) which IMO does not reflect the QuarkusTestProfile at all.>
I know. Sorry for bringing this up here, I understand it may be confusing. I brought this up because recently, I've also encountered issues with io. quarkus.test.junit.QuarkusUnitTest#getConfigOverrides. They have very similar names, and in my opinion, they should behave in the same way, no matter which testing context you are in.

No problem. You mean QuarkusTestProfile#getConfigOverrides(), right? Well, these are two completely separate APIs. So the names are similar but the use cases are very different.

Currently, all of these config properties are backed by the same ConfigSource - the one used to load application.properties. Which is probably not a problem because test authors control the deployment structure.

Yes, I know. We could split each API into its own source with its own ordinality, making it clear who overrides who.

I would personally start with a minimum viable fix and javadoc clarification. And we could improve the things later 🤷.