-
Notifications
You must be signed in to change notification settings - Fork 555
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
4.x: Enable microprofile/tests/tck/tck-config/src/test/tck-suite.xml tests #8173
base: main
Are you sure you want to change the base?
Conversation
146f4fb
to
82c58c7
Compare
…tests helidon-io#8173 Signed-off-by: Jorge Bescos Gascon <jorge.bescos.gascon@oracle.com>
} catch (NoSuchElementException e) { | ||
// Property expression does not resolve | ||
return Optional.empty(); | ||
return Optional.of(new ConfigValueImpl(propName, null, rawValue, name, ordinal)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the point 1 I was referring before.
@@ -313,35 +334,47 @@ private <T> T convert(String propertyName, Class<T> type, String value) { | |||
} | |||
} | |||
|
|||
private Optional<ConfigValue> findConfigValue(String propertyName) { | |||
private Optional<ConfigValue> findConfigValue(String propertyName, Optional<String> profiledPropertyName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we do not use optional parameters. Also it does not make sense in this case, as you wrap it just to unwrap it later. Please use null
here, as it is a private method (null
is not allowed in public API only).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
String rawValue = value; | ||
String name = source.getName(); | ||
int ordinal = source.getOrdinal(); | ||
final String propName = selectedProperty; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This name is not descriptive. Use selectedPropertyFinal
or something similar, with a comment "required final variable, as it is used in lambdas"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -126,70 +129,88 @@ public <T> T getValue(String propertyName, Class<T> propertyType) { | |||
@SuppressWarnings("unchecked") | |||
@Override | |||
public <T> Optional<T> getOptionalValue(String propertyName, Class<T> propertyType) { | |||
String profiledPropertyName = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This statement does not make sense here - the next if is checking for profile being null. So just move the content of the if statement after the if checking if profile is null and remove the check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
+ " in source " + source.getName() | ||
+ " and it is empty (removed)"); | ||
} | ||
return Optional.empty(); | ||
return Optional.of(new ConfigValueImpl(propName, null, rawValue, name, ordinal)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this not returning an empty optional? I am missing an explanation of this change (and once more further down).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this particular case you are right, it can return empty Optional, so I have reverted it.
For the other case:
[ERROR] Failures:
[ERROR] CDIPropertyExpressionsTest>Arquillian.run:138->badExpansion:84 expected [test] but found [null]
[ERROR] PropertyExpressionsTest>Arquillian.run:138->noExpressionButConfigValue:145 expected [test] but found [null]
[ERROR] PropertyExpressionsTest>Arquillian.run:138->noExpressionComposedButConfigValue:171 expected [test] but found [null]
I am guided by the test results and then stepping back from there to see what is causing the failure. The tests do not give any explication, so I am afraid I cannot tell you why it has to behave in this way.
0ebf6f6
to
e84e8cd
Compare
Signed-off-by: Jorge Bescos Gascon <jorge.bescos.gascon@oracle.com>
7a17219
to
6dc63dd
Compare
Signed-off-by: Jorge Bescos Gascon <jorge.bescos.gascon@oracle.com>
@tomas-langer kindly reminder to review this. |
#6105
Description
We were waiting for microprofile-config 3.1 to enable one test. This version made other issues that are fixed in this PR too.
There were 2 types of issues:
Documentation
N/A