From d9fa7084453167ba017df95b4b10337e0cf829cc Mon Sep 17 00:00:00 2001 From: amrali-eg <32075105+amrali-eg@users.noreply.github.com> Date: Thu, 10 Sep 2026 01:37:18 +0300 Subject: [PATCH 1/3] Decide cancellation in one place, keeping the error that explains it RequestCancel and WaitForStoppedConversion split one decision across two methods that only worked in one order, and the second waited for a condition the first had already established - so its timeout message could never appear. They are now one wait: find the button or a final status, press once, and judge from what the window reports. The press failure is no longer thrown away. A refusal certainly delivered nothing, so it still escapes to the retry loop. Any other automation failure might have followed a press that landed, so pressing stops - but the exception is now kept and named in the failure, because it is the likeliest explanation of a run that then finishes uncancelled. Without it that outcome was indistinguishable from a machine too fast to interrupt. Simulating an RPC failure on the press, the previous code reported only "Cancellation was not exercised. EC instead reported: Conversion complete: 1000 converted"; it now adds "The press to Cancel failed with an unknown outcome and was not repeated: COMException: simulated RPC failure". Controls: that simulation names the cause where master does not, five refusals followed by acceptance still cancel and interrupt the run, and a press that lands and then throws still passes. Phase I keeps its own "Conversion stopped" assertion as an independent check. EC is unchanged. Co-Authored-By: Claude Opus 5 --- .../EncodingChecker.GuiSmoke/EcGuiDriver.cs | 113 ++++++++---------- 1 file changed, 52 insertions(+), 61 deletions(-) diff --git a/sources/EncodingChecker.GuiSmoke/EcGuiDriver.cs b/sources/EncodingChecker.GuiSmoke/EcGuiDriver.cs index b966f75..4fb1f58 100644 --- a/sources/EncodingChecker.GuiSmoke/EcGuiDriver.cs +++ b/sources/EncodingChecker.GuiSmoke/EcGuiDriver.cs @@ -245,105 +245,96 @@ internal void ProceedThenCancel(AutomationElement review, Func writingHasB () => writingHasBegun() || ConversionHasFinished(), "The conversion neither began writing nor reported that it had finished."); - RequestCancel(); - - WaitForStoppedConversion(); + CancelAndConfirmStopped(); } /// - /// Requests cancellation, or fails if the run finishes before it can be cancelled. + /// Cancels the run and requires the window to report that it was stopped. /// /// - /// The window hides the button when a run ends - MainForm sets - /// btnCancel.Visible - so an absent button has two meanings: the run beat the - /// phase to it, or automation failed to see a button that is on screen. Only the - /// window's own final status separates them. Either meaning prevents this phase from - /// proving cancellation, so neither is accepted as a successful test. - /// - /// The button and final status are raced so a fast completion produces a clear failure - /// instead of a misleading timeout. - /// - /// Whether cancellation happened is not decided here. Delivering a click is not - /// proof it took effect, and failing to deliver one is not proof it did not: the - /// window's own final status is the only thing that separates a run that was stopped - /// from one that finished on its own, and - /// reads it. This method's job is to ask, and to wait until the run has reported - /// something. + /// One wait decides everything, because the three things that could be asked + /// separately - has a button appeared, did the press land, what did the run report - + /// are only meaningful together. Pressing is not proof of cancelling: the window + /// hides the button when a run ends, so a press can fail precisely because it + /// worked, and an absent button says only that some run is over. The status is the + /// single fact that separates a run that was stopped from one that finished on its + /// own, and EC writes "Conversion stopped" for the first and "Conversion complete" + /// for the second. /// - /// The two ways a click can fail mean different things. A refusal - the control - /// reporting itself not-enabled - happens before anything is delivered, so trying - /// again is safe and right, and it is left to the shared retry loop, which repeats it - /// and keeps it as the cause a timeout would name. - /// - /// Any other automation failure might have followed a click that did land: an element - /// disappearing mid-call is what a successful cancel looks like when the window hides - /// the button in response. Clicking again there would be a fresh action rather than a - /// retry, so the attempt stops and the status is left to say what happened. + /// The press is attempted once. A refusal - the control reporting itself + /// not-enabled - is the one failure that certainly delivered nothing, so it is left + /// to the retry loop, which repeats it and keeps it as a cause. Any other automation + /// failure might have followed a press that did land, so pressing stops there and + /// the exception is kept: it is the likeliest explanation of a run that then + /// finishes uncancelled, and without it that outcome is indistinguishable from a + /// machine too fast to interrupt. /// - private void RequestCancel() + private void CancelAndConfirmStopped() { - bool clickMayHaveLanded = false; + bool pressed = false; + Exception? uncertainPress = null; - WaitUntil( + string? finalStatus = WaitFor( () => { - // Once a click may be in flight, stop pressing the button and just watch. - if (!clickMayHaveLanded) + if (!pressed) { AutomationElement? cancel = FindById(MainWindow, "btnCancel"); - // Gone means the window hid it, which it does only when a run ends - - // but that has to come from the status, not the button's absence. if (cancel is not null) { try { Invoke(cancel); - clickMayHaveLanded = true; + pressed = true; } - // A refusal is the one failure that certainly delivered nothing, - // and it is deliberately not caught: the shared loop retries it - // and keeps it, so a wait that expires on repeated refusals can - // name them. Answering "not ready" here would clear that cause. - // - // Every other automation failure might have followed a click that - // landed, so the attempt stops and the status is left to say. catch (Exception ex) when ( ex is not ElementNotEnabledException && ex is ElementNotAvailableException or COMException or InvalidOperationException) { - clickMayHaveLanded = true; + uncertainPress = ex; + pressed = true; } } } - return ConversionHasFinished(); + return StatusLine() is string status && IsFinalConversionStatus(status) + ? status + : null; }, - "The run never reported a final status after cancellation was requested."); - } - - /// Requires the cancellation request to produce an interrupted run. - private void WaitForStoppedConversion() - { - string? finalStatus = null; + Timeout, + out Exception? lastError); - WaitForOperationOutcome( - () => - { - finalStatus = StatusLine(); - return finalStatus is not null && IsFinalConversionStatus(finalStatus); - }, - "The conversion did not report a final result after cancellation."); + if (finalStatus is null) + { + throw Expired( + (pressed + ? "Cancel was pressed but the run never reported a final status." + : "No Cancel button appeared and the run never reported a final status.") + + Blame(uncertainPress), + lastError); + } - if (!finalStatus!.Contains("Conversion stopped", StringComparison.Ordinal)) + if (!finalStatus.Contains("Conversion stopped", StringComparison.Ordinal)) { throw new GuiDriverException( - "Cancellation was not exercised. EC instead reported: " + finalStatus); + "Cancellation was not exercised. EC instead reported: " + finalStatus + + Blame(uncertainPress)); } } + /// + /// Names the failed press when there was one, so a run that finished uncancelled is + /// not reported as a machine that was simply too fast. + /// + private static string Blame(Exception? uncertainPress) => + uncertainPress is null + ? string.Empty + : " The press to Cancel failed with an unknown outcome and was not repeated: " + + $"{uncertainPress.GetType().Name}: {uncertainPress.Message}"; + /// Waits until the status line contains . /// /// The window enables its buttons before it assigns the final status, so a run that From bccb0e6ca7d60bc4a2b22c93f37e54f11d60b8ca Mon Sep 17 00:00:00 2001 From: amrali-eg <32075105+amrali-eg@users.noreply.github.com> Date: Thu, 10 Sep 2026 01:51:11 +0300 Subject: [PATCH 2/3] Name a refused press, not just an uncertain one A definite refusal is retried, so it never reaches uncertainPress - it ends up in the wait's retained error instead. When refusals ran until the conversion finished on its own, the failure appended nothing and read as a machine that was merely too fast, which is the outcome this evidence exists to rule out. Blame now takes both and words them differently, because they are different evidence: an uncertain press may have landed and was deliberately not repeated; a refusal certainly delivered nothing and was retried for as long as the run lasted. The timeout path passes only the uncertain press, since Expired already names whatever the wait was still retrying. Simulating a button that refuses every press, the failure now ends "The last attempt to press Cancel was refused: ElementNotEnabledException: The operation is not allowed on a nonenabled element."; before it stopped at EC's tally. The remarks no longer claim the press is attempted once. At most one press with an uncertain outcome is attempted; a press that was definitely refused may be retried. EC is unchanged. Co-Authored-By: Claude Opus 5 --- .../EncodingChecker.GuiSmoke/EcGuiDriver.cs | 47 ++++++++++++++----- 1 file changed, 35 insertions(+), 12 deletions(-) diff --git a/sources/EncodingChecker.GuiSmoke/EcGuiDriver.cs b/sources/EncodingChecker.GuiSmoke/EcGuiDriver.cs index 4fb1f58..d550a52 100644 --- a/sources/EncodingChecker.GuiSmoke/EcGuiDriver.cs +++ b/sources/EncodingChecker.GuiSmoke/EcGuiDriver.cs @@ -261,13 +261,16 @@ internal void ProceedThenCancel(AutomationElement review, Func writingHasB /// own, and EC writes "Conversion stopped" for the first and "Conversion complete" /// for the second. /// - /// The press is attempted once. A refusal - the control reporting itself + /// At most one press with an uncertain outcome is attempted. A press that was + /// definitely refused may be retried: a refusal - the control reporting itself /// not-enabled - is the one failure that certainly delivered nothing, so it is left /// to the retry loop, which repeats it and keeps it as a cause. Any other automation /// failure might have followed a press that did land, so pressing stops there and - /// the exception is kept: it is the likeliest explanation of a run that then - /// finishes uncancelled, and without it that outcome is indistinguishable from a - /// machine too fast to interrupt. + /// the exception is kept. + /// + /// Either way the reason is named in the failure. A run that finishes uncancelled + /// after a press that was refused, or one whose outcome was unknown, is otherwise + /// indistinguishable from a machine too fast to interrupt. /// private void CancelAndConfirmStopped() { @@ -309,11 +312,13 @@ or COMException if (finalStatus is null) { + // Only the uncertain press is added here: Expired already names whatever + // the wait was still retrying, which is where a refusal shows up. throw Expired( (pressed ? "Cancel was pressed but the run never reported a final status." : "No Cancel button appeared and the run never reported a final status.") - + Blame(uncertainPress), + + Blame(uncertainPress, null), lastError); } @@ -321,19 +326,37 @@ or COMException { throw new GuiDriverException( "Cancellation was not exercised. EC instead reported: " + finalStatus - + Blame(uncertainPress)); + + Blame(uncertainPress, lastError)); } } /// - /// Names the failed press when there was one, so a run that finished uncancelled is + /// Names why the press did not stop the run, so a run that finished uncancelled is /// not reported as a machine that was simply too fast. /// - private static string Blame(Exception? uncertainPress) => - uncertainPress is null - ? string.Empty - : " The press to Cancel failed with an unknown outcome and was not repeated: " - + $"{uncertainPress.GetType().Name}: {uncertainPress.Message}"; + /// + /// The two are different evidence and read differently. An uncertain press may have + /// landed and was deliberately not repeated. A refusal certainly delivered nothing + /// and was retried for as long as the run lasted, and arrives as + /// - the error the wait was still holding, which it + /// keeps only when the refusal was the most recent thing to happen. + /// + private static string Blame(Exception? uncertainPress, Exception? lastRefusal) + { + if (uncertainPress is not null) + { + return " The Cancel press failed with an unknown outcome and was not repeated: " + + $"{uncertainPress.GetType().Name}: {uncertainPress.Message}"; + } + + if (lastRefusal is ElementNotEnabledException) + { + return " The last attempt to press Cancel was refused: " + + $"{lastRefusal.GetType().Name}: {lastRefusal.Message}"; + } + + return string.Empty; + } /// Waits until the status line contains . /// From 9719eeaa36426b22eceeb42cae21f3afaeeae556 Mon Sep 17 00:00:00 2001 From: amrali-eg <32075105+amrali-eg@users.noreply.github.com> Date: Thu, 10 Sep 2026 01:55:58 +0300 Subject: [PATCH 3/3] Record the phase A timeout nobody can explain yet One full-suite run in thirteen failed phase A with "EncodingChecker did not return to its idle state", and the diagnostic showed only the window's chrome - no status bar text, no result rows. It did not reproduce: phase A alone passed twelve of twelve on that build and twelve of twelve on master, and the suite passed eight of eight afterwards. It is not attributable to the cancellation work being validated at the time. Phase A cancels a review and never enters that path, and master carries the same WaitForMainReady, so the flake most likely predates it. EC-28 records where to look: WaitForMainReady still waits for any final status rather than evidence of the action just performed, which is EC-26's shape in a helper that fix did not reach; the timeout cannot say whether the process is alive, the window handle valid, the review gone, or the status bar findable; and the driver holds one AutomationElement for the main window from startup, which would fail every later read if it went stale while the window was healthy. Reproduction should run the whole suite, since the phase alone does not show it. Recorded rather than waited out: a gate that fails for reasons nobody can name is the problem EC-24, EC-26 and EC-27 all turned out to be. Co-Authored-By: Claude Opus 5 --- docs/DEFECT-BACKLOG.md | 47 ++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 45 insertions(+), 2 deletions(-) diff --git a/docs/DEFECT-BACKLOG.md b/docs/DEFECT-BACKLOG.md index a046676..a3b7e20 100644 --- a/docs/DEFECT-BACKLOG.md +++ b/docs/DEFECT-BACKLOG.md @@ -4,9 +4,9 @@ This is the current ledger for defects and review findings in EncodingChecker. It is organised by status, not discovery date, so the open work is visible in one place. Longer evidence and history follow the ledger. - + -**Derived count: 66 findings — 55 fixed, 7 open, 1 not reproduced, 1 withdrawn, +**Derived count: 67 findings — 55 fixed, 8 open, 1 not reproduced, 1 withdrawn, 1 intentional behavior, and 1 design decision.** Recompute and check these figures with: @@ -52,6 +52,7 @@ the 2026-09-08 reformat to findings that previously had only a sentence. |---|---|---|---|---|---| | EC-17 | A comment describes the text check backwards | Open | Low | Common | [EC-17](#ec-17) | | EC-20 | A file can change while EC is detecting its encoding | Open | Low | Rare | [EC-20](#ec-20) | +| EC-28 | Phase A once timed out waiting for the window to go idle | Open | Medium | Rare | [EC-28](#ec-28) | | CX-06 | EC checks for random-looking data before checking for a BOM | Open | Medium | Theoretical | [CX-06](#cx-06) | | BL-05 | Force-closing during a conversion can produce an error on exit | Open | Low | Rare | [BL-05](#bl-05) | | BL-19 | ASCII with many NUL bytes can be reported as UTF-16 | Open | Medium | Rare | [BL-19](#bl-19) | @@ -1049,6 +1050,48 @@ early. EC-26 was later fixed by removing the remaining enabled-button checks fro readiness decisions. The driver now waits for evidence produced by the operation itself. +### EC-28 + +**Phase A failed once with `TimeoutException: EncodingChecker did not return to +its idle state`, and the cause is not known.** It happened on 2026-09-10 in one +full-suite run out of thirteen, while validating the cancellation state machine. + +`WaitForMainReady` waits for the review window to be gone and for the status to +show a final conversion result. The diagnostic it printed listed only the window's +chrome - `File Encoding Checker | System Menu Bar | System | Minimize | Maximize | +Close` - with no status bar text and no result rows among it, so at that moment +the driver could see the window frame but nothing inside it. + +**It did not reproduce.** Phase A alone passed twelve times out of twelve on the +build that failed, and twelve out of twelve on `master`. The full suite passed +eight times out of eight afterwards. Isolated runs may simply be the wrong shape +to catch it: the failure appeared in a sequence where nine other phases had +already driven the same window. + +**It is not attributable to the change being validated.** Phase A cancels a review +and never enters the cancellation path that change rewrote, and the patch was +checked to have removed only the two methods it intended to remove. `master` +carries the same `WaitForMainReady`, so the flake most likely predates it. + +Where to look, in the order that would settle it fastest: + +- `WaitForMainReady` waits for *any* final conversion status rather than evidence + of the action just performed. That is [EC-26](#ec-26)'s shape reappearing in a + helper the fix did not reach, and it is recorded there as still open. +- The timeout reports what the window showed but not whether the process is still + alive, whether the main-window handle is still valid, whether the review is + genuinely gone, or whether `statusBar` can be found at all. A failure that + cannot distinguish those is hard to diagnose from CI alone. +- The driver holds the `AutomationElement` for the main window from startup. If + that element goes stale, every later read fails while the window is perfectly + healthy; reacquiring it on failure would tell the two apart. +- Reproduction should run the whole suite rather than the phase alone. + +Until then this is one unexplained red on a required check. It is recorded rather +than waited out because a gate that fails for reasons nobody can name is the +problem this project keeps returning to - [EC-24](#ec-24), [EC-26](#ec-26) and +[EC-27](#ec-27) are all the same story, and each of them looked like noise first. + ## Decisions and mistakes that must remain visible ### A known defect shipped after being reported closed