feature/TOOL-654-dont-rerun-full-test-suite by jwoodwardcertinia · Pull Request #89 · apex-dev-tools/test-runner

jwoodwardcertinia · 2026-09-02T20:48:38Z

Summary

Fix 1 (TOOL-654): AsyncTestRunner.prepareRestart no longer falls back to a full re-run when a reset finds no pending classes. Previously, if every class in the queue had reached a terminal status but the run was still stalled, the runner assumed it couldn't tell what was incomplete and resubmitted the entire suite. In production this meant a single missing result (5 out of ~40k tests, caused by the org closing a class's queue item before all its results were persisted) triggered a full ~25-minute re-run. Now that case returns the partial result directly and defers to Testall.asyncRun's existing missing-test detection, which finds and re-runs only the specific absent tests. The three possible outcomes of prepareRestart are now modeled as a PrepareRestartResult discriminated union (restart / allComplete / unknown) rather than overloading undefined.

Fix 2: TestRunCancelAborter.abortRun now confirms a cancellation actually took effect org-side before returning, instead of firing the abort DML and returning immediately. Without this, a caller that resubmits tests for the same classes right after an abort (e.g. Testall's missing-test rerun) could race the still-live original run and hit ALREADY_IN_PROCESS: Test already enqueued. It now polls ApexTestQueueItem until no pending items remain, using the previously-unused CancelTestRunOptions/getCancelPollInterval/getCancelPollTimeout and Logger.logWaitingForCancel (leftovers from an equivalent check that had been removed). It's best-effort: on timeout it logs a warning and returns rather than blocking the caller.

Notes for consumers

AsyncTestRunner is exported directly, and when a run stalls with nothing left in the queue it now returns a partial result whose run.Status is still Processing and with no error set — where previously it re-ran the full suite. Driven through Testall this is handled: missing-test detection runs, and the follow-up run's status replaces it. If nothing turns out to be missing there is no follow-up run, so TestRunSummary.runResult.Status stays Processing. This doesn't reach the generated reports — ReportGenerator derives the outcome from the test results and never reads Status. The pre-existing "stalled before it started processing" path added in 3.4.0 already returns a non-terminal status the same way.

When a stalled run defers to missing-test detection, those missing tests are now subject to maxErrorsForReRun like any other Testall re-run. Previously the internal restart re-ran pending classes regardless of the failure count, so with more than maxErrorsForReRun genuine failures the missing tests will now be left absent rather than chased — logged as Aborting missing test check as N failed - max re-run limit exceeded. This is intentional: the limit exists to stop spending org time on a run that is already failing.

Testing

Lots of new jest test cases.
Apart from that, it's a bit of a faff to manually test. I did manage to get an abort and reset in the middle of a run. You can see the new logging as it polled.

00:01:00 [Processing] Passed: 9 | Failed: 0 | 9/1549 Complete (0%) | No progress 4/5
00:01:05 [Processing] Passed: 9 | Failed: 0 | 9/1549 Complete (0%) | No progress 5/5
Test run '707RL00001ewDMZ' was not progressing, cancelling and retrying...
Reset 1/2 before abandoning run
Reusing 9 tests from 4 completed classes; rerunning 1540 remaining tests across 158 classes
Cancelling test run '707RL00001ewDMZYA2'
Test run '707RL00001ewDMZYA2' has been cancelled
Test run started with AsyncApexJob Id: 707RL00001ew2Fe
00:00:00 [Queued] Passed: 0 | Failed: 0 | 0/1540 Complete (0%) | No progress 1/5
00:00:05 [Processing] Passed: 3 | Failed: 0 | 3/1540 Complete (0%)
00:00:11 [Processing] Passed: 29 | Failed: 0 | 29/1540 Complete (1%)

kjonescertinia

Review summary

The core logic of both fixes looks correct and the new specs exercise the main new paths. I didn't find correctness bugs in the reset/restart flow. Requesting changes (minor) for: one behavioural change in the aborter that now affects the user-cancel and run-timeout paths with a long default wait, one untested error branch in the confirmation poll, and two files the PR leaves unformatted. Details are in the inline comments; a few items that don't map to a diff line are listed below.

Verdict: Request changes (minor). Happy to approve once the two Medium items are addressed or consciously deferred.

Local verification (head `2e00c99`)

Step	Result
`pnpm install`	OK
`pnpm build`	OK
`eslint ./src` (check only)	Clean
`prettier --check src test`	7 files flagged; 2 are new to this PR (`src/runner/TestRunCancelAborter.ts`, `test/runner/TestRunCancelAborter.spec.ts`), the other 5 were already unformatted on main
`pnpm test`	104 tests, all pass (one spec, `TestResultMatcher.spec.ts`, only failed under my sandbox because it writes `../.apexTestRerun`; it passes normally and is untouched by this PR)

CI only runs install/build/test and eslint doesn't enforce prettier, so the formatting items won't fail CI.

Checks that passed

numberOfResets is still incremented on the allComplete path and asserted in both new specs.
When maxTestRunRetries is exhausted, restart stays unknown, control falls through to runInternal, which throws the max-retries TestError exactly as on main. The existing spec should abort and abandon without re-running when retries are exhausted still passes.
Already-complete run: abortRun does no DML and the confirmation poll returns on the first query; the existing should cancel when no tests still running spec covers it.
CancelTestRunOptions, getCancelPollInterval, getCancelPollTimeout and logWaitingForCancel already existed on main following the getX(options) pattern; the PR only wires them in. No changes to src/index.ts.
No console.log; all logging goes through Logger.

Test coverage

restart: existing specs. allComplete: two new specs. unknown: existing spec. Cancel poll success / timeout: two new specs.
Gaps: the confirmation-poll query-error branch (pollRetryIf, line 120 of the aborter) is never executed; there is no Testall-level test that a runner result with run.Status === 'Processing' and no error flows into missing-test detection.

Items not tied to a diff line

Deferring the allComplete case to Testall means those missing tests are now subject to maxErrorsForReRun (Testall.asyncRun), whereas an internal restart was not. Probably desirable, but worth a line in the PR description since it's a change from main.
cancelPollTimoutMins (typo, Timout) in TestOptions.ts predates this PR, but this PR makes the option functional for the first time, so this is the cheapest moment to add a correctly spelled name alongside it.
Commit message on the second commit: "Pool after aborting" should be "Poll".

kjonescertinia · 2026-09-02T22:08:36Z

      }
    }

+    await this.waitForCancelConfirmation(logger, connection, testRunId, options);


Medium: the confirmation wait now applies to every abort path, not only restarts.

The comment below (lines 90-93) says the goal is to avoid ALREADY_IN_PROCESS on resubmission. That only matters for the hang-restart path and the allComplete/stuck paths that hand off to Testall. It's wasted on the two other callers in TestRunner.runInternal: the run-timeout abort (~line 193) and the caller's cancellation-token abort (~line 198), where nothing is resubmitted.

With defaults, the first re-poll is after 30s and the timeout is 10 minutes, so a user pressing cancel can now wait much longer for run() to resolve than before. Suggest either a flag (on abortRun or CancelTestRunOptions) so those two call sites skip confirmation, or documenting the change and lowering the default timeout.

Good idea. I will add an option for this into TestOptions

kjonescertinia · 2026-09-02T22:08:36Z

+      },
+
+      pollUntil: outstandingCount => outstandingCount === 0,
+      pollRetryIf: () => true,


Medium: a persistent SOQL failure here spins until the full timeout.

pollRetryIf: () => true means an expired session or network failure is retried every 30s for 10 minutes (logging "Poll failed" each time) before the best-effort warning fires. Coverage shows this branch is never executed by the specs.

Either bail early on non-transient errors, or at least add a spec for the query-throws case so the intended behaviour is pinned down.

Changed to pollRetryIf: () => false so we don't retry at all. If there is an error checking if the abort finished we'll try to restart and we're basically where we were before anyway.

kjonescertinia · 2026-09-02T22:08:36Z

+          await this.abortTestRun(result.run.AsyncApexJobId);
+          result.numberOfResets = this._stats.getNumberOfTimesReset();
+          this._logger.logRunCompleteMissingResults(testRunIdResult.testRunId);
+          return result;


Low: Processing can leak into the final report / public API.

This returns a result whose run.Status is still Processing with no error. Testall handles it correctly (TestResultStore.hasAborted only checks Aborted, so missing-test detection runs). But if the missing set turns out empty (all results did arrive, the run just never flipped status) nothing overwrites the status and the summary reports a finished run as Processing.

Also, AsyncTestRunner is public via src/index.ts, so direct consumers can now receive a non-terminal status with no error where they previously got a re-run. Worth a note in the PR description, and ideally a Testall-level spec covering this handoff.

I agree. Adding test cases and updating the PR description.

kjonescertinia · 2026-09-02T22:08:36Z

+        // restart here. Whether results actually are missing is Testall's
+        // call: its missing-test check compares expected vs. persisted
+        // results and re-runs anything absent.
+        return { kind: 'allComplete' };


Low (behavioural nuance): by deferring here, the missing tests become subject to maxErrorsForReRun in Testall.asyncRun, whereas an internal restart re-ran pending classes regardless of the failure count. Likely the desired behaviour, just flagging it as a change from main so it's a conscious decision.

I have updated the PR description so we are explicit about this. I don't think we want to change how it works. I don't think missing tests are very common actually.

kjonescertinia · 2026-09-02T22:08:36Z

+      logRegex(`Cancelling test run '${testRunId}'`)
+    );
+    expect(logger.entries[1]).to.match(
+      logRegex(`Waiting for test run '${testRunId}' to cancel... 1 tests queued`)


Low: prettier. This file (lines 150 and 184-186) and src/runner/TestRunCancelAborter.ts:71 fail prettier --check; both were clean on main. Not enforced by CI or eslint, so purely housekeeping.

kjonescertinia · 2026-09-02T22:08:36Z

+          this._stats = this._stats.reset();
+          await this.abortTestRun(result.run.AsyncApexJobId);
+          result.numberOfResets = this._stats.getNumberOfTimesReset();
+          this._logger.logRunCompleteMissingResults(testRunIdResult.testRunId);


Nit (log ordering): this explanatory message is logged after the abort, so in the output it appears after "Cancelling test run" / "has been cancelled". Logging it before abortTestRun reads more naturally.

I don't think it's worth trying to change this.

kjonescertinia · 2026-09-02T22:08:36Z

-import { retry } from './Poll';
+import { Pollable, poll, retry } from './Poll';
+
+const PENDING_STATUSES = "'Holding', 'Queued', 'Preparing', 'Processing'";


Simplification: this hardcoded string duplicates PENDING_QUEUE_STATUSES in TestRunner.ts. Exporting one array from src/model/ApexTestQueueItem.ts and building the IN (...) clause from it would keep the two in sync.

I can't imagine SF adding a new status to the apex test runner. But you never know.

kjonescertinia · 2026-09-02T22:08:36Z


+    await this.waitForCancelConfirmation(logger, connection, testRunId, options);
+
    logger.logRunCancelled(testRunId);


Nit: on the timeout path this logs "has been cancelled" immediately after the "Could not confirm ... finished cancelling" warning, which reads as contradictory. Consider a different message (or skipping this one) when confirmation failed.

That makes sense. Changed.

…ke sense.

…cuteAnonymous to reduce repeated code.

…ed but failed to do so.

jwoodwardcertinia added 2 commits September 2, 2026 15:15

Handle when a test run hangs right at the end

9b7938b

Pool after aborting a run until it actually aborted in the org

2e00c99

jwoodwardcertinia requested review from nawforce and pwrightcertinia September 2, 2026 20:48

kjonescertinia requested changes Sep 2, 2026

View reviewed changes

jwoodwardcertinia added 8 commits September 3, 2026 11:24

Add confirmation option when aborting, and skip it when it doesn't ma…

760ba5d

…ke sense.

Update changelog

2bef6b4

Don't retry if the poll fails after an abort. And update the mock exe…

1918059

…cuteAnonymous to reduce repeated code.

Add more test scenarios for stalled runs

5cd340d

Simplify code and fix comment

1cb30f0

Reuse pending statuses.

fae8159

Don't log that we cancelled the run if we tried to confirm it cancell…

ff92f5a

…ed but failed to do so.

Fix some long lines

292b7eb

kjonescertinia approved these changes Sep 3, 2026

View reviewed changes

jwoodwardcertinia merged commit 2f26124 into main Sep 3, 2026
1 check passed


		await this.waitForCancelConfirmation(logger, connection, testRunId, options);

		logger.logRunCancelled(testRunId);

Uh oh!

Conversation

jwoodwardcertinia commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Notes for consumers

Testing

Uh oh!

kjonescertinia left a comment

Choose a reason for hiding this comment

Review summary

Local verification (head 2e00c99)

Checks that passed

Test coverage

Items not tied to a diff line

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jwoodwardcertinia Sep 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

jwoodwardcertinia commented Sep 2, 2026 •

edited

Loading

Local verification (head `2e00c99`)

jwoodwardcertinia Sep 3, 2026 •

edited

Loading