Restore node-agent support (#29) and GitHub-checks publisher (#30) onto master#32
Merged
Merged
Conversation
PRs #29 and #30 were merged into the build-report-embed branch, but that branch had already been squash-merged to master as #28 beforehand, so their code never reached master. This brings the stranded changes forward, on top of the current master (which already has #28 and the #31 inline-media feature): - #29: run the freestyle tunnel on the build's agent (TunnelManager.startOnChannel/ stopOnChannel + shared registry; wrapper + testingbotTunnel step delegate to it). - #30: TestingBotChecksPublisher (testingbotChecks) publishing results as a GitHub check, plus the checks-api + display-url-api dependencies and docs. No behavior change versus the already-reviewed #29/#30; only rebased onto master. The inline-media (#31) files are left untouched.
📝 WalkthroughWalkthroughThe plugin adds a GitHub Checks publisher and documentation, plus channel-aware TestingBot tunnel lifecycle management with build-scoped identifiers, agent-side registries, controller fallback, and teardown handling. ChangesAgent-aware tunnel lifecycle
GitHub Checks publishing
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/testingbot/pipeline/TestingBotTunnelStep.java (1)
186-198: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftMove tunnel startup off the CPS VM thread and make aborts during startup tear down the tunnel.
startOnChannel()runs beforebodyis assigned, so an abort during startup leavesstop()with nothing to cancel and can leak the tunnel. Move startup to a nonblocking path and close the tunnel on cancellation, including the race after startup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/testingbot/pipeline/TestingBotTunnelStep.java` around lines 186 - 198, Update the tunnel startup flow around TestingBotTunnelStep.startOnChannel and the body invocation so startup does not block the CPS VM thread. Track startup cancellation and ensure stop() can cancel an in-progress startup; when startup completes after cancellation, immediately invoke stopTunnelQuietly for the created tunnel, preserving cleanup for body-invocation failures and normal callback teardown.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Around line 157-161: Update the README example to invoke testingbotChecks
after junit, preferably within an always post-build block, so published checks
include sessions attached by the JUnit publisher even when tests fail.
In `@src/main/java/testingbot/TestingBotChecksPublisher.java`:
- Around line 98-110: The session-processing logic around
TestingBotChecksPublisher should aggregate outcomes for duplicate session IDs
before determining the final conclusion. Replace the first-occurrence counting
and row creation with per-session state where any failed case marks that session
failed, then calculate total/passed counts and generate rows from the aggregated
results after all cases are processed.
- Around line 132-147: The TestingBot report link added by the publisher must
not expose the authentication hash through GitHub checks. Update the report-link
construction around TestingBotBuildReportAction.getReportUrl() and the
ChecksDetails withDetailsURL(detailsUrl(run, reportAction)) flow to use a
Jenkins-controlled or token-free URL, while preserving the existing report-link
behavior otherwise.
In `@src/main/java/testingbot/TunnelManager.java`:
- Line 13: Replace the process-local AtomicInteger-based TUNNEL_SEQ identifier
generation in TunnelManager with UUID-based tunnel identifiers. Update the
tunnel creation path around the affected lines to generate a fresh UUID for each
tunnel, removing the sequencing dependency while preserving the existing
identifier usage and format expectations.
- Around line 250-258: Update stopOnChannel’s null-channel branch to avoid
removing tunnelIdentifier from RUNNING_TUNNELS, since the offline agent registry
cannot be modified from the controller. Preserve the existing offline warning
and early return, allowing any controller-local tunnel with the same identifier
to remain managed.
---
Outside diff comments:
In `@src/main/java/testingbot/pipeline/TestingBotTunnelStep.java`:
- Around line 186-198: Update the tunnel startup flow around
TestingBotTunnelStep.startOnChannel and the body invocation so startup does not
block the CPS VM thread. Track startup cancellation and ensure stop() can cancel
an in-progress startup; when startup completes after cancellation, immediately
invoke stopTunnelQuietly for the created tunnel, preserving cleanup for
body-invocation failures and normal callback teardown.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e3c379f5-ee3f-40da-8373-0a311f0b71ef
📒 Files selected for processing (9)
README.mdpom.xmlsrc/main/java/testingbot/TestingBotBuildWrapper.javasrc/main/java/testingbot/TestingBotChecksPublisher.javasrc/main/java/testingbot/TunnelManager.javasrc/main/java/testingbot/pipeline/TestingBotTunnelStep.javasrc/main/resources/testingbot/TestingBotChecksPublisher/config.jellysrc/main/resources/testingbot/TestingBotChecksPublisher/help-message.htmlsrc/main/resources/testingbot/TestingBotChecksPublisher/help-name.html
Comment on lines
+157
to
+161
| Add the **Publish TestingBot results as a GitHub check** post-build action (freestyle), or call the `testingbotChecks` step in a pipeline: | ||
|
|
||
| ```groovy | ||
| testingbotChecks(name: 'TestingBot', message: 'End-to-end tests on TestingBot') | ||
| ``` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Show testingbotChecks after junit.
The publisher only reads sessions already attached through AbstractTestResultAction. Calling this example before junit finds no sessions and can publish a success fallback. Document it after junit—typically in an always post block—so failed test runs still publish the correct check.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` around lines 157 - 161, Update the README example to invoke
testingbotChecks after junit, preferably within an always post-build block, so
published checks include sessions attached by the JUnit publisher even when
tests fail.
Comment on lines
+98
to
+110
| boolean casePassed = cr.isPassed(); | ||
| for (String sessionId : TestingBotReportFactory.findSessionIDs(cr)) { | ||
| String id = sessionId == null ? "" : sessionId.trim(); | ||
| // Count each distinct TestingBot session once, even if it appears in several | ||
| // cases or a case logs several sessions; cases with no sessions add nothing. | ||
| if (id.isEmpty() || !seenSessions.add(id)) { | ||
| continue; | ||
| } | ||
| total++; | ||
| if (casePassed) { | ||
| passed++; | ||
| } | ||
| rows.add("| " + cr.getFullName() + " | " + (casePassed ? "✅ passed" : "❌ failed") + " |"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Aggregate duplicate session outcomes before choosing the conclusion.
Lines 101-104 intentionally allow one session to occur in several cases, but the first case wins. If that occurrence passed and a later case for the same session failed, the failure is discarded and this check can incorrectly conclude SUCCESS. Track each session’s aggregate result (failure should dominate) and generate rows after aggregation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/testingbot/TestingBotChecksPublisher.java` around lines 98 -
110, The session-processing logic around TestingBotChecksPublisher should
aggregate outcomes for duplicate session IDs before determining the final
conclusion. Replace the first-occurrence counting and row creation with
per-session state where any failed case marks that session failed, then
calculate total/passed counts and generate rows from the aggregated results
after all cases are processed.
Comment on lines
+132
to
+147
| TestingBotBuildReportAction reportAction = run.getAction(TestingBotBuildReportAction.class); | ||
| if (reportAction != null) { | ||
| text.append("\n[View the full TestingBot build report](").append(reportAction.getReportUrl()).append(")\n"); | ||
| } | ||
|
|
||
| ChecksOutput output = new ChecksOutput.ChecksOutputBuilder() | ||
| .withTitle(getName()) | ||
| .withSummary(summary) | ||
| .withText(text.toString()) | ||
| .build(); | ||
|
|
||
| ChecksDetails details = new ChecksDetails.ChecksDetailsBuilder() | ||
| .withName(getName()) | ||
| .withStatus(ChecksStatus.COMPLETED) | ||
| .withConclusion(conclusion) | ||
| .withDetailsURL(detailsUrl(run, reportAction)) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Do not publish the TestingBot report auth token to GitHub.
TestingBotBuildReportAction.getReportUrl() includes ?auth=<authHash> (src/main/java/testingbot/TestingBotBuildReportAction.java, Lines 90-92). Lines 134 and 147 place that authenticated URL in GitHub-hosted check fields, exposing report access to anyone permitted to view the repository’s checks. Use a Jenkins-controlled or token-free URL instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/testingbot/TestingBotChecksPublisher.java` around lines 132 -
147, The TestingBot report link added by the publisher must not expose the
authentication hash through GitHub checks. Update the report-link construction
around TestingBotBuildReportAction.getReportUrl() and the ChecksDetails
withDetailsURL(detailsUrl(run, reportAction)) flow to use a Jenkins-controlled
or token-free URL, while preserving the existing report-link behavior otherwise.
| import java.util.List; | ||
| import java.util.Map; | ||
| import java.util.concurrent.ConcurrentHashMap; | ||
| import java.util.concurrent.atomic.AtomicInteger; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Use restart-safe tunnel identifiers.
TUNNEL_SEQ resets after a controller restart. A resumed Pipeline can then reuse an identifier still active for the same build on an agent, causing duplicate rejection or targeting the wrong tunnel. Use a UUID instead of process-local sequencing.
Proposed fix
-import java.util.concurrent.atomic.AtomicInteger;
+import java.util.UUID;
- /** Monotonic suffix so multiple generated identifiers within one run stay distinct. */
- private static final AtomicInteger TUNNEL_SEQ = new AtomicInteger();
-
public static String generateTunnelIdentifier(String jobFullName, int buildNumber) {
- return "jenkins-" + jobFullName.replace('/', '-') + "-" + buildNumber + "-" + TUNNEL_SEQ.incrementAndGet();
+ return "jenkins-" + jobFullName.replace('/', '-') + "-" + buildNumber + "-" + UUID.randomUUID();
}Also applies to: 224-233
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/testingbot/TunnelManager.java` at line 13, Replace the
process-local AtomicInteger-based TUNNEL_SEQ identifier generation in
TunnelManager with UUID-based tunnel identifiers. Update the tunnel creation
path around the affected lines to generate a fresh UUID for each tunnel,
removing the sequencing dependency while preserving the existing identifier
usage and format expectations.
Comment on lines
+250
to
+258
| public static void stopOnChannel(VirtualChannel channel, String tunnelIdentifier, TaskListener listener) { | ||
| if (channel == null) { | ||
| RUNNING_TUNNELS.remove(tunnelIdentifier); | ||
| if (listener != null) { | ||
| listener.getLogger().println("[TestingBot] Agent offline: remote teardown of tunnel " | ||
| + tunnelIdentifier + " could not be performed. It may still be running on the agent " | ||
| + "and should stop when the agent process exits (best-effort cleanup)."); | ||
| } | ||
| return; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not remove an agent-owned entry from the controller registry.
With a null channel, Line 252 runs in the controller JVM and cannot affect the offline agent’s registry. It can instead remove a same-named controller-local tunnel without stopping its App, permanently orphaning that tunnel.
Proposed fix
if (channel == null) {
- RUNNING_TUNNELS.remove(tunnelIdentifier);
if (listener != null) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public static void stopOnChannel(VirtualChannel channel, String tunnelIdentifier, TaskListener listener) { | |
| if (channel == null) { | |
| RUNNING_TUNNELS.remove(tunnelIdentifier); | |
| if (listener != null) { | |
| listener.getLogger().println("[TestingBot] Agent offline: remote teardown of tunnel " | |
| + tunnelIdentifier + " could not be performed. It may still be running on the agent " | |
| + "and should stop when the agent process exits (best-effort cleanup)."); | |
| } | |
| return; | |
| public static void stopOnChannel(VirtualChannel channel, String tunnelIdentifier, TaskListener listener) { | |
| if (channel == null) { | |
| if (listener != null) { | |
| listener.getLogger().println("[TestingBot] Agent offline: remote teardown of tunnel " | |
| tunnelIdentifier + " could not be performed. It may still be running on the agent " | |
| "and should stop when the agent process exits (best-effort cleanup)."); | |
| } | |
| return; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/main/java/testingbot/TunnelManager.java` around lines 250 - 258, Update
stopOnChannel’s null-channel branch to avoid removing tunnelIdentifier from
RUNNING_TUNNELS, since the offline agent registry cannot be modified from the
controller. Preserve the existing offline warning and early return, allowing any
controller-local tunnel with the same identifier to remain managed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
PRs #29 (run the tunnel on the build's agent) and #30 (
testingbotChecksGitHub-check publisher) were merged withbase=build-report-embed. But #28 wasbuild-report-embed, and it had already been squash-merged intomasterbefore #29/#30 landed in it — so their code went into thebuild-report-embedbranch and never reachedmaster.masteris therefore missing node-agent support and the checks publisher, even though both PRs show as "merged". This restores them.What
Brings the stranded #29 + #30 changes forward onto current
master(which already has #28 and the #31 inline-media feature):TunnelManager.startOnChannel/stopOnChannel+ sharedRUNNING_TUNNELSregistry;TestingBotBuildWrapperruns the tunnel via the agent channel and re-resolves it at teardown;testingbotTunnelstep delegates to the shared helper.TestingBotChecksPublisher(testingbotChecksstep + freestyle post-build action) with configurablename/message, unique-session counting, and the safe-no-opchecks-apipublish; addschecks-api+display-url-apideps and README/help docs.The #31 inline-media files (
TestingBotBuildObject,summary.jelly,index.jelly) are left as-is on master.Note
This is exactly the already-reviewed #29/#30 code (including all their CodeRabbit fixes), just re-based onto master — the only new integration surface is the
pom.xmldeps and theREADME.mdsection, both merged cleanly with #31's content.Validation
mvn -DskipTests verify(SpotBugs on) clean on the combined tree; all three features (#29 tunnel-on-agent, #30 checks publisher, #31 media) confirmed present in the built artifact.Summary by CodeRabbit
New Features
testingbotChecksstep.Documentation