diff --git a/sources/EncodingChecker.GuiSmoke/EcGuiDriver.cs b/sources/EncodingChecker.GuiSmoke/EcGuiDriver.cs index b0336fb..b966f75 100644 --- a/sources/EncodingChecker.GuiSmoke/EcGuiDriver.cs +++ b/sources/EncodingChecker.GuiSmoke/EcGuiDriver.cs @@ -76,7 +76,7 @@ private void RequireDefaultTarget() { string target = TargetEncoding(); - if (!target.Equals("utf-8", StringComparison.OrdinalIgnoreCase)) + if (!string.Equals(target, "utf-8", StringComparison.OrdinalIgnoreCase)) { throw new GuiDriverException( $"The target encoding opens on '{target}', not 'utf-8'. Every phase " @@ -261,43 +261,67 @@ internal void ProceedThenCancel(AutomationElement review, Func writingHasB /// 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. If neither appears, the wait retains the last - /// automation error. + /// 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. + /// + /// 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. /// private void RequestCancel() { - AutomationElement? cancel = null; + bool clickMayHaveLanded = false; WaitUntil( () => { - cancel = FindById(MainWindow, "btnCancel"); - return cancel is not null || ConversionHasFinished(); + // Once a click may be in flight, stop pressing the button and just watch. + if (!clickMayHaveLanded) + { + 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; + } + // 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; + } + } + } + + return ConversionHasFinished(); }, - "The run offered neither a Cancel button nor a final status."); - - if (cancel is null) - { - throw new GuiDriverException( - "The conversion finished before cancellation could be exercised."); - } - - try - { - Invoke(cancel); - } - catch (Exception ex) when ( - ex is ElementNotEnabledException or ElementNotAvailableException) - { - // Confirm why the button vanished, but do not count ordinary completion as a - // cancellation test. - WaitForOperationOutcome( - () => ConversionHasFinished(), - "Cancel became unavailable without the run reporting that it had stopped."); - - throw new GuiDriverException( - "The conversion finished before cancellation could be exercised."); - } + "The run never reported a final status after cancellation was requested."); } /// Requires the cancellation request to produce an interrupted run. @@ -595,7 +619,7 @@ private void SelectCombo( { AutomationElement combo = RequireById(root, automationId); - if (SelectedName(combo).Equals(value, StringComparison.OrdinalIgnoreCase)) + if (string.Equals(SelectedName(combo), value, StringComparison.OrdinalIgnoreCase)) return; if (!combo.TryGetCurrentPattern(ValuePattern.Pattern, out object? rawValue)) @@ -612,7 +636,7 @@ private void SelectCombo( setter.SetValue(value); WaitUntil( - () => SelectedName(combo).Equals(value, StringComparison.OrdinalIgnoreCase), + () => string.Equals(SelectedName(combo), value, StringComparison.OrdinalIgnoreCase), $"'{value}' was not selected in '{automationId}'."); } @@ -624,11 +648,14 @@ private static string SelectedName(AutomationElement combo) ((SelectionPattern)rawSelection).Current.GetSelection(); if (selected.Length > 0) - return selected[0].Current.Name; + return selected[0].Current.Name ?? string.Empty; } + // A provider may hand back null for either of these. Absorbing it here means + // callers can compare the result without guarding, and an unreadable selection + // fails their assertion rather than their null check. if (combo.TryGetCurrentPattern(ValuePattern.Pattern, out object? rawValue)) - return ((ValuePattern)rawValue).Current.Value; + return ((ValuePattern)rawValue).Current.Value ?? string.Empty; return string.Empty; } @@ -842,8 +869,10 @@ private static AutomationElement WaitForElement( ?? throw Expired(timeoutMessage, lastError); /// - /// The last error retried before giving up. A probe that threw every time is the - /// likeliest reason a wait expired, and it is what a bare timeout cannot report. + /// The last error retried, when the probe was still failing at the end. A poll that + /// answers cleanly clears it, so this reports the cause of a wait that kept throwing + /// rather than one that simply never became true - which is the case a bare timeout + /// cannot explain by itself. /// There is deliberately no overload without it, so a wait that reports a timeout /// cannot leave the cause out by accident. Only a caller with nothing to report /// discards it, and no wait in this driver currently does. diff --git a/sources/EncodingChecker.GuiSmoke/SmokeSuite.cs b/sources/EncodingChecker.GuiSmoke/SmokeSuite.cs index 9c52a50..60e65a3 100644 --- a/sources/EncodingChecker.GuiSmoke/SmokeSuite.cs +++ b/sources/EncodingChecker.GuiSmoke/SmokeSuite.cs @@ -177,13 +177,20 @@ private void PhaseA(PhaseContext phase) // otherwise drives that control. Exercise it here, where the run is over and this // phase is about to prove no bytes moved: a target that could not be changed would // pass every other phase unnoticed. + // Read once and report that same reading: Check takes a message that is built + // whether or not it fails, so asking the window again inside it would drive the + // control on every passing run and could describe a value the assertion never + // tested. Compared the way the driver compares it, so the two cannot disagree + // over a control label's casing. gui.SetTargetEncoding("us-ascii"); - Check(gui.TargetEncoding() == "us-ascii", - $"The target encoding did not change: {gui.TargetEncoding()}"); + string changed = gui.TargetEncoding(); + Check(string.Equals(changed, "us-ascii", StringComparison.OrdinalIgnoreCase), + $"The target encoding did not change: {changed}"); gui.SetTargetEncoding("utf-8"); - Check(gui.TargetEncoding() == "utf-8", - $"The target encoding did not change back: {gui.TargetEncoding()}"); + string restored = gui.TargetEncoding(); + Check(string.Equals(restored, "utf-8", StringComparison.OrdinalIgnoreCase), + $"The target encoding did not change back: {restored}"); AssertSameFiles(before, Snapshot(directory)); AssertNoArtifacts(directory); @@ -422,7 +429,7 @@ private void PhaseI(PhaseContext phase) using var gui = new EcGuiDriver(_app); System.Windows.Automation.AutomationElement review = gui.OpenReview(directory, count); - gui.ProceedThenCancel(review, () => RewrittenCount(directory) >= 1); + gui.ProceedThenCancel(review, () => AnyRewritten(directory)); int rewritten = RewrittenCount(directory); int untouched = count - rewritten; @@ -511,7 +518,18 @@ private void PhaseJ(PhaseContext phase) AssertNoArtifacts(phase.Directory); } - /// Files whose byte-order mark has been stripped, so they were written. + /// Whether EC has rewritten anything yet. + /// + /// Asked every 50 ms while the conversion is running, so it stops at the first + /// rewritten file rather than counting them all. Counting opens every file in the + /// directory on each probe, which delays the cancellation this triggers and eats the + /// margin phase I depends on - the count is wanted once, after the run has stopped. + /// + private static bool AnyRewritten(string directory) => + Directory.EnumerateFiles(directory, "file-*.txt") + .Any(path => !StartsWithUtf8Bom(path)); + + /// How many files EC rewrote. Taken after the run, never during it. private static int RewrittenCount(string directory) => Directory.EnumerateFiles(directory, "file-*.txt") .Count(path => !StartsWithUtf8Bom(path));