Introduce RemoveSomethingStepTest - #362
Conversation
Signed-off-by: Jim Klimov <jimklimov+jenkinsci@gmail.com>
daniel-kraemer
left a comment
daniel-kraemer
left a comment
There was a problem hiding this comment.
No objections against adding more tests and coverage.
If you can share a reproducer for the initial issue you are having, I could help pin down the root cause?
|
Wrong account…anyways, the offering still stands 😅 |
|
I think the root cause was that part of our library still referred to And there I see that For comparison, The weird thing for me is that the saved references to these badges do remain valid, strings can be changed etc. - just that with their Action detached from the Build, they no longer visualize there. |
As discovered during investigation that led to jenkinsci/badge-plugin#362 this plugin's `removeBadges()` action actually removes both badges and summaries (is not limited to specifically `BadgeAction`). There are a couple of ways out of this: * Define a new method name for the new activity = least surprise for existing consumers that might rely on this (mis-)behavior. * Accept that this was a bug and redefine `removeBadges()` to remove only badges = least surprise for consumers of the current Badge API plugin, and more intuitively matching the name. * If anyone wanted a way to remove any derivative of `AbstractBadgeAction` in one go, a separate method specifically for that can be used, again more intuitively fitting. This would be effectively about swapping the names and updating javadocs of methods impacted by this PR's initial commit. For now, I went with the first option, but consider the second one superior (albeit a slightly breaking change, potentially). Up to maintainers :)
|
Fired an educated guess for GPB Plugin at jenkinsci/groovy-postbuild-plugin#197 |
3 participants
Cheers, I am troubleshooting the mysterious case of a disappearing summary in my pipeline library (a placeholder entry is added, seen on the build page, but later disappears instead of being updated - maybe due to a
removeBadges()with noidconstraint between job phases).To confirm or rule this out, the proposed test was added (passes both ways on my local build at least, so probably my library problem lurks elsewhere), and now I hope the tested cases make sense in the upstream as a measure against future regression in this area.
I think they are more specific for this question than the existing
RemoveBadgesStepTestand its descendantRemoveSummariesStepTest- which together form a smart construct, I concede, but did not leave me convinced that they test not only that the steps do the work requested, but also do not have unrequested side impacts. Maybe they do, lost somewhere in the web ofassertActionExists(run, expected);checks, while my code spells out the calls and expectations unambiguously :-D