Extend MicroProfileConfig default-value validation #510
Conversation
|
Your PR looks promising! Could you sign please eca. I think you should adjust some tests or add new tests. |
Validating a default value can succeed, fail, or be unsupported Signed-off-by: E14 <[email protected]>
E14
force-pushed
the
default-values-no-fail-on-unsupported-types
branch
from
90b8f20 to
81884bb
Compare
Done!
Fully agree - I mostly created this PR to use the CI because master is not stable (😑). Give me a bit though, I took too long today getting a spammable mailbox that both eclipse and github accept so I can connect the ECA Glad the basic idea looks good to you though! |
Signed-off-by: E14 <[email protected]>
Signed-off-by: E14 <[email protected]>
| d1, d2, d3, d4, d5); | ||
| } | ||
|
|
||
| @Test |
There was a problem hiding this comment.
This test (and others that are built similar) is unstable for me (and it seems gh CI) - sometimes it doesn't report any diagnostics.
I'm assuming there is issue resulting from an async result taking just a tiny bit too long, I don't know how to avoid it other than changing the assertion helper method to use busy waiting like awaitility does. That would however only work reliably with at least one diagnostic, and also cause higher CPU load during tests (tests are already being aborted by CI due to taking too long)
There was a problem hiding this comment.
So, I dug a bit (a lot) deeper into this issue yesterday, and it seems it cannot be resolved by retrying.
I was able to narrow it down to "somewhere before" MicroProfileConfigASTValidator#isAdaptedForDiagnostics, because when this method returns false (meaning when JDTTypeUtils.findType(javaProject, CONFIG_PROPERTY_ANNOTATION) returns null), it will always do so for the test run. This leads me to believe it's related to project loading (but the project data seems fine)
At that point, I'm out - I hope you can agree that this is well out of scope for this PR and stability issues of the test have nothing to do with my changes.
|
I also thought it was a big project, but since we have more issues related to this super annoying problem, I spent all day yesterday working on this idea and I managed to run the converters from a microprofile application like Quarkus in a POC project that has nothing to do with LSP4MP. I'm currently integrating my POC, and if it works well, I hope it will be available next week. I'm sorry to have made you work for nothing, I'm really sorry -( If my idea doesn't work, I'll merge your PR. I'll keep you posted. |
2 participants
Validating a default value can succeed, fail, or be unsupported.
Partly resolves / mitigates #396
Before:
After:
