Fix WorkflowActionsFactory to check both successful and failed builds…#382
Fix WorkflowActionsFactory to check both successful and failed builds…#382aviral282 wants to merge 3 commits intojenkinsci:masterfrom
Conversation
… for HTML reports - Refactor logic to separately handle build-level reports (keepAll=true) from last successful build - Add support for project-level markers (keepAll=false) from last build even if failed - Add comprehensive test coverage for WorkflowActionsFactory with various build scenarios - Improve code clarity with better variable names and documentation Signed-off-by: Race, Chris <chris.race@fmr.com>
MarkEWaite
left a comment
MarkEWaite
left a comment
There was a problem hiding this comment.
Thanks very much for the pull request. Please merge with the upstream master branch so that you can include the most recent changes. The plugin has switched from JUnit 4 based tests to JUnit 5. That will require some changes in your automated test.
…prove assertion messages Signed-off-by: Race, Chris <chris.race@fmr.com>
Hello @MarkEWaite I have the made the latest changes and they are tested on our end now |
|
Hello @MarkEWaite Can this be reviewed and merged? we have added tested and rebased with latest changes. |
MarkEWaite
left a comment
MarkEWaite
left a comment
There was a problem hiding this comment.
The changes look good to me. I'm not a maintainer of this plugin. I don't have permission to merge the pull request. That will need to wait for a review and merge by one of the maintainers of the plugin.
olamy
left a comment
olamy
left a comment
There was a problem hiding this comment.
LGTM
maybe wait 1/2 more days and we can merge.
Thanks for the tests
| // If reports are being saved on the build level (keep for all builds) | ||
| List<HtmlPublisherTarget.HTMLBuildAction> reports = r.getActions(HtmlPublisherTarget.HTMLBuildAction.class); | ||
| final Run<?,?> lastSuccessful = j.getLastSuccessfulBuild(); | ||
| final Run<?,?> lastBuild = j.getLastBuild(); |
There was a problem hiding this comment.
Are you sure you want this and not lastCompleteBuild?
lastBuild may still be running so reports may not be finalized.
There was a problem hiding this comment.
lastBuild definitely seems wrong.
As noted in Jira, the current behavior is not a mistake, and so while there may be a use case in a particular job to permit reports to be collected from a failing build, this should probably be made opt-in.
There was a problem hiding this comment.
Hello @jtnord & @jglick
Thanks for the feedback
The code uses:
getLastSuccessfulBuild() - gets the last successful build only
getLastBuild() - gets the very last build (could be failed, running, or any status)
This is intentional because:
For keepAll=true reports: You want to show reports from the last successful build
For keepAll=false reports: You want to show reports from the last build even if it failed (which is what the PR is fixing)
Given this, it is my opinion that the logic should stay as is a the keepAll flag will determine if it's the last successful or just the last build.
There was a problem hiding this comment.
You want to show reports from the last build even if it failed
Well, no, not in general, as I tried to explain in Jira.
!keepAll would be that you only need to display at the job level one report from the most recent successful (= stable or unstable) build and so there is no need to retain reports from prior successful builds. Failed (or aborted or “not built”) builds or running builds do not have a definitive outcome for any particular report as the build was terminated prematurely. (You might look at a report while diagnosing the failure but it would not be representative of the job as a whole, whose status is simply “broken”.)
Your specific use case seems to be that you have a pipeline which might successfully produce a report in one stage but then fail (fatally) in a subsequent stage, yet you wish to treat that build outcome as a kind of mixture of some reports and a partial foreseeable failure. Normally this is handled by making the build run to completion but be marked unstable in that stage (for example with the catchError step, or even junit combined with a tool like Bats etc.), but pending a refactoring of this kind it may be helpful in a particular job to display reports from a failed build under such a condition. My recommendation stands that such behavior be opt-in as it is contrary to the normal way job-level reports are handled.
There was a problem hiding this comment.
@jglick Thanks for the feedback
We realize that our initial respond was assuming that the job should be showing a representation of the current job status (with report failures), but after reading your response it does seem to make more sense that the job shows the last successful report as we don't know where in the job the one may have failed.
Therefore, if we update the plugin to show the last successful instead, would that satisfy?
If someone down the road disagrees, then they can contribute an opt-in flag to show the last build regardless of success/failure.
There was a problem hiding this comment.
update the plugin to show the last successful instead
Is that not what is does already?
There was a problem hiding this comment.
We are trying to Fix the Broken Link when the Reports Does not shows up in the Jenkins along with the additional changes
There was a problem hiding this comment.
Oh I was not aware of a broken link. If true, that is clearly a bug.
There was a problem hiding this comment.
@jglick @jtnord @olamy
Thanks for your feedback, We will test this more to address the changes and we are covering all the scenarios of Multibranch/Standalone/Freestyle Pipelines to analyze the behavior.
I'll try to keep either change this PR to draft while I actively work in these or keep it open for some more time to finalize the changes.
6 participants
For issue - https://issues.jenkins.io/browse/JENKINS-76148
This pull request improves the logic for collecting HTML report actions in workflow jobs and adds comprehensive unit tests for the
WorkflowActionsFactory. The main change ensures that build-level reports are collected from the last successful build, while project-level reports are collected from the last build, regardless of its result. Additionally, the new test suite thoroughly verifies the behavior for various job and build scenarios.Improvements to report collection logic:
WorkflowActionsFactory.createFor(Job j)to separately collect build-level HTML reports from the last successful build and project-level reports from the last build, ensuring both types are included appropriately.Testing done
WorkflowActionsFactoryTest.javatest suite with multiple tests covering non-workflow jobs, jobs with no builds, jobs with successful and failed builds, and edge cases like null builds and class name matching.Submitter checklist