Quarkus LTS Upgrade to 3.8.4 #3477
Conversation
| return topicName.orElse(channelName); | ||
| } | ||
|
|
||
| Iterable<String> getPropertyNames() { |
There was a problem hiding this comment.
I think this method and getOptionalValue should be protected rather than package
There was a problem hiding this comment.
why protected and not default?
There was a problem hiding this comment.
Sorry, I forgot to mention why ;).
It is just styling. I prefer to use package for methods that might be called only within the package and protected for methods that might be inherited.
It is true that package is more restrictive (a protected can be inherited outside the original package) and you are only overriding from the unit test within the same package, theferore both are fine.
There was a problem hiding this comment.
For example, when I see the change initially, I believe you have missed the private (later I see you are inheriting in the test), if you put protected, it is clear the purpose of change.
There was a problem hiding this comment.
as this is in the personal preference realm, I'll leave as is as my personal preference is default scope as it's more restrictive and supposed to be used only by unit test to avoid using environment config
| } | ||
| }; | ||
|
|
||
| Iterable<String> getPropertyNames() { |
| }; | ||
|
|
||
| Iterable<String> getPropertyNames() { | ||
| return properties.keySet(); |
There was a problem hiding this comment.
same here, actually, to avoid code duplication (which I agree in test is mostly irrelevant) this probably deserve an inner class, TestQuarkusTopicDiscovery, with a constructor that accept a Map. Also, for an unmodifiable Map of reduced size like this one, Map.of should be used rather than the map initializer construct.
There was a problem hiding this comment.
I don't think code duplication is necessarily bad in unit tests, but I can adjust no problem.
| <dependency> | ||
| <groupId>org.junit.jupiter</groupId> | ||
| <artifactId>junit-jupiter</artifactId> | ||
| <scope>compile</scope> |
There was a problem hiding this comment.
hmmm, are we completely sure that kogito-test-utils is only added as test scoped? if not, we might add with junit being added to set of the dependencies accidentally (probably it is all right, since we would have already realized that assert and rest assure were accidentally being added and it does not seem to be the case)
There was a problem hiding this comment.
Actually, why JUnit has to be added to the dependency set? Or the other way around, how this util test was working without this dependency?
There was a problem hiding this comment.
I can't remember what I had to add this, I'll try remove and see how it goes.
| } | ||
| }; | ||
| } | ||
|
|
There was a problem hiding this comment.
@fjtirado improved test code
…ve the issue anymore.
… for test failures.
* Upgrade to and align with Quarkus 3.8.2 LTS release. * Upgrade to and align with Quarkus 3.8.3 LTS release. * Upgrade to and align with Quarkus 3.8.4 LTS release. * Upgrade to and align with Quarkus 3.8.4 LTS release. * Improved test code to avoid duplication. * Remove unnecessary util class execution, as Quarkus 3.8.x does not have the issue anymore. * Upgrade tolatest embedded postgresql that is supposed to have the fix for test failures.
Related apache/incubator-kie-issues#958
Ensemble:
apache/incubator-kie-drools#5853
https://github.com/apache/incubator-kie-optaplanner/pull/3076/files
#3477
apache/incubator-kie-kogito-apps#2039
apache/incubator-kie-kogito-examples#1908
apache/incubator-kie-kogito-images#1760