Fix export of list attributes with custom Stapler converters#2789
Fix export of list attributes with custom Stapler converters#2789somiljain2006 wants to merge 17 commits intojenkinsci:masterfrom
Conversation
|
Approach looks rights but coverage is too low and tests need improving. This will also need a run through the bom repo to see if it breaks any tests in other repositories. |
|
@timja I’ve added additional tests to cover the supported collection and array export paths introduced by this change. Could you please take another look and let me know if the coverage is sufficient now? |
|
Coverage is still not great: https://github.com/jenkinsci/configuration-as-code-plugin/pull/2789/checks?check_run_id=64384731677 |
|
@timja I’ve added tests covering the supported and intended scenarios for the new logic, including export and configuration of List, Set, and array, iteration over collection elements (including null entries). The 28% coverage reported by CI seems to stem from the fact that the unreached lines are defensive fallbacks, which I haven't been able to trigger via standard testing. Specifically, the completely uncovered lines (315-336, 340-346, 349) and partially covered branches (309-312, 339, 348, 350) exist to handle raw arrays or malformed data. However, due to strict YAML validation, it fails earlier and raises a ConfiguratorException before these paths can be reached. So, is there a way that is actually intended to hit these defensive branches, or how would you prefer I handle this unreachable code? |
| } | ||
|
|
||
| @SuppressWarnings("unused") | ||
| public static class StaplerConverterImpl implements Converter { |
There was a problem hiding this comment.
This converter doesn't seem to be doing anything, I deleted it and the tests still passed.
I would expect this to actually do something and have the tests rely on the behaviour.
There was a problem hiding this comment.
That StaplerConverterImpl was a leftover from my earlier attempts to increase coverage. I have completely removed the unused converter in the latest commit.
There was a problem hiding this comment.
isn't the whole point of this to have a customer converter though and demonstrate that it gets called for each item in the list rather than the list gets passed to it?
Remove them then if they aren't reachable then they are misleading |
| Object converted; | ||
|
|
||
| if (a.isMultiple() && value instanceof Collection) { | ||
| List<Object> list = new ArrayList<>(); |
There was a problem hiding this comment.
no coverage of this.
Can you please run the coverage locally?
mvnd clean verify jacoco:report -P enable-jacoco
# then
open plugin/target/site/jacoco/index.html
here's a screenshot of the current state:
There was a problem hiding this comment.
Yeah, I'm working on adding tests to get those exact lines covered, and thanks for sharing that local coverage command that is going to make testing this locally much easier
|
I think there is something wrong with my logic because only if statements' conditions are being covered, not the inner logic, so I'm making this pr draft for a while till i came up with a new logic. |
2 participants
Fix export of list attributes with custom Stapler converters by ensuring per-item conversion is applied during DataBoundConfigurator export, rather than passing collections directly to Stapler. This prevents incorrect mapping output and avoids argument type mismatches.
Fixes #2346
Your checklist for this pull request
🚨 Please review the guidelines for contributing to this repository.