From 8bcd96a0c81cf7f3db2c563f357651fa6207bb2f Mon Sep 17 00:00:00 2001 From: amrali-eg <32075105+amrali-eg@users.noreply.github.com> Date: Thu, 10 Sep 2026 02:57:40 +0300 Subject: [PATCH] One way to do each of the things the driver was doing several ways Cleanup only; behaviour is unchanged everywhere except one message that is now built later than it was. Reading rendered text was written out three times - for the review, the whole window, and the status bar - identical but for the root element. One VisibleText holds it, and the three callers keep the doc comments that say why each wants a different root. The select-all wait built its failure message eagerly, so two full walks of a thousand result rows ran on every passing call, and an automation call sat unguarded inside a message. WaitUntil gained an overload taking the message as a function, evaluated only on failure and routed through Safely. That is the same hazard the suite documents for its own Check. "Has the run ended" had two spellings, one named and one inlined in the cancel probe; FinalConversionStatus now answers it once and ConversionHasFinished is the null test over it. ResultCount hand-rolled what ResultItems already does, including an is-var pattern that always matched. The safe status read was inlined at three failure sites and now has the name its sibling DescribeWindowsSafely already had. SetToggle's confirmation refetched the toggle pattern each poll instead of reading the one it holds. Two comments described code that is no longer there: the idle-wait remark still warned about the defect the parameter below it fixed, and an inline comment restated the remark six lines above it. Not done, and why: caching the status bar or results list element would remove a whole-window walk from every 50 ms poll, but a stale held element is exactly the hypothesis EC-28 is open on, so adding more of them now would be reckless. Provider-side ControlType filtering and a narrower scope for the review lookup would both be cheaper, but they change UIA query semantics on the timing- sensitive path whose margin is a tracked number. EC is unchanged. Co-Authored-By: Claude Opus 5 --- .../EncodingChecker.GuiSmoke/EcGuiDriver.cs | 105 +++++++++--------- 1 file changed, 50 insertions(+), 55 deletions(-) diff --git a/sources/EncodingChecker.GuiSmoke/EcGuiDriver.cs b/sources/EncodingChecker.GuiSmoke/EcGuiDriver.cs index 31c6acd..9752b23 100644 --- a/sources/EncodingChecker.GuiSmoke/EcGuiDriver.cs +++ b/sources/EncodingChecker.GuiSmoke/EcGuiDriver.cs @@ -105,9 +105,9 @@ private AutomationElement OpenSelectedReview(int expectedFiles) SetToggle(MainWindow, "chkSelectDeselectAll", true); WaitUntil( () => CheckedResultCount() == expectedFiles, - $"Select all did not check {expectedFiles} result row(s). " + - $"Observed {CheckedResultCount()} checked row(s). " + - DescribeResultItems()); + () => $"Select all did not check {expectedFiles} result row(s). " + + $"Observed {CheckedResultCount()} checked row(s). " + + DescribeResultItems()); Invoke(MainWindow, "btnConvert"); return WaitForReview(); @@ -200,10 +200,13 @@ internal bool ReviewContainsControl(AutomationElement review, string automationI /// prove the label exists. What matters is the wording a reader actually sees, so /// this reads the rendered text rather than a control's presence. /// - internal string ReviewText(AutomationElement review) => + internal string ReviewText(AutomationElement review) => VisibleText(review); + + /// Every non-blank name under an element, joined one per line. + private static string VisibleText(AutomationElement root) => string.Join( "\n", - review.FindAll(TreeScope.Descendants, Condition.TrueCondition) + root.FindAll(TreeScope.Descendants, Condition.TrueCondition) .Cast() .Select(element => element.Current.Name) .Where(name => !string.IsNullOrWhiteSpace(name))); @@ -215,13 +218,7 @@ internal string ReviewText(AutomationElement review) => /// alone would not. Waiting is a different job - see , which /// reads only the status bar and does not throw. /// - internal string StatusText() => - string.Join( - "\n", - MainWindow.FindAll(TreeScope.Descendants, Condition.TrueCondition) - .Cast() - .Select(element => element.Current.Name) - .Where(name => !string.IsNullOrWhiteSpace(name))); + internal string StatusText() => VisibleText(MainWindow); /// /// Starts the conversion and cancels it once the status bar shows progress. @@ -238,9 +235,8 @@ internal void ProceedThenCancel(AutomationElement review, Func writingHasB Invoke(review, "btnProceedConversion"); WaitUntil(() => !WindowExists(handle), "The conversion review did not close."); - // Timed against real progress rather than a sleep, so the phase does not depend - // on how fast the machine converts. Cancelling before the first write would - // exercise the declined-review path instead, which phase A already covers. + // Cancelling before the first write would exercise the declined-review path + // instead, which phase A already covers. WaitForOperationOutcome( () => writingHasBegun() || ConversionHasFinished(), "The conversion neither began writing nor reported that it had finished."); @@ -285,7 +281,7 @@ private void CancelAndConfirmStopped() // at. Progress would then depend on the button disappearing rather than // on the run reporting, and a button that lingered after the run ended // would keep being refused with the answer already on screen. - if (StatusLine() is string status && IsFinalConversionStatus(status)) + if (FinalConversionStatus() is string status) return status; if (!pressAttempted) @@ -414,9 +410,7 @@ internal void WaitForStatus(string fragment) /// any final conversion status. A status outlives the action that wrote it - the /// window clears it only when the next action starts - so accepting any of them lets /// a wait be satisfied by the previous action's report and return before the current - /// one has finished. Every phase drives one action per window today, which is the - /// only reason that has not bitten; it is the shape EC-26 was about, in the one - /// helper that fix did not reach. + /// one has finished, so each caller names the headline its own action produces. /// private void WaitForMainReady(string expectedHeadline) { @@ -434,8 +428,7 @@ private void WaitForMainReady(string expectedHeadline) throw Expired( $"EncodingChecker did not go idle: no '{expectedHeadline}' was reported." - + Safely(() => " The status showed: " + StatusText(), - " The status could not be read") + + DescribeStatusSafely() + DescribeIdleState(), lastError); } @@ -493,8 +486,7 @@ private void WaitForOperationOutcome(Func evidence, string what) } throw Expired( - what + Safely(() => " The status showed: " + StatusText(), - " The status could not be read"), + what + DescribeStatusSafely(), lastError); } @@ -525,12 +517,7 @@ private bool ScanHasFinished() => if (bar is null) return null; - return string.Join( - "\n", - bar.FindAll(TreeScope.Descendants, Condition.TrueCondition) - .Cast() - .Select(element => element.Current.Name) - .Where(name => !string.IsNullOrWhiteSpace(name))); + return VisibleText(bar); } catch (Exception ex) when ( ex is ElementNotAvailableException or InvalidOperationException or COMException) @@ -539,19 +526,19 @@ private bool ScanHasFinished() => } } + private bool ConversionHasFinished() => FinalConversionStatus() is not null; + + /// + /// The status a finished run reported, or null while one is still running. + /// + private string? FinalConversionStatus() => + StatusLine() is string status && IsFinalConversionStatus(status) ? status : null; + /// /// Every way a conversion or preview can end writes one of these, including the paths /// where nothing was modified. Matching the headline rather than the counts keeps this /// independent of what the run actually did. /// - private bool ConversionHasFinished() - { - if (StatusLine() is not string status) - return false; - - return IsFinalConversionStatus(status); - } - private static bool IsFinalConversionStatus(string status) => status.Contains("Conversion complete", StringComparison.Ordinal) || status.Contains("Conversion stopped", StringComparison.Ordinal) || @@ -590,6 +577,10 @@ private AutomationElement WaitForReview(int previousHandle = 0) /// important one. A listing that fails says why, alongside that error rather than /// instead of it. /// + private string DescribeStatusSafely() => + Safely(() => " The status showed: " + StatusText(), + " The status could not be read"); + private string DescribeWindowsSafely() => Safely(() => " EC exposed these windows: " + DescribeTopLevelWindows(), " The windows could not be listed either"); @@ -616,20 +607,10 @@ private static string Safely(Func describe, string whenItFails) FindProcessElementById("ConversionConfirmationForm") ?? FindProcessElementByTitle("Review conversion"); - private int ResultCount() - { - AutomationElement? list = FindById(MainWindow, "lstResults"); - - if (list is null) - return 0; - - AutomationElementCollection children = list.FindAll( - TreeScope.Children, Condition.TrueCondition); - - return children.Cast().Count(element => - element.Current.ControlType is var type && - (type == ControlType.DataItem || type == ControlType.ListItem)); - } + private int ResultCount() => + FindById(MainWindow, "lstResults") is AutomationElement list + ? ResultItems(list).Count() + : 0; private void SetRefusedFileChecked( AutomationElement review, @@ -772,8 +753,7 @@ private void SetToggle( toggle.Toggle(); WaitUntil( - () => ((TogglePattern)element.GetCurrentPattern(TogglePattern.Pattern)) - .Current.ToggleState == (value ? ToggleState.On : ToggleState.Off), + () => toggle.Current.ToggleState == (value ? ToggleState.On : ToggleState.Off), $"'{automationId}' did not reach the requested state."); } @@ -1011,7 +991,20 @@ private static AutomationElement WaitForElement( return null; } - private static void WaitUntil(Func predicate, string timeoutMessage) + private static void WaitUntil(Func predicate, string timeoutMessage) => + WaitUntil(predicate, () => timeoutMessage); + + /// + /// Waits, building the failure message only if there is a failure to describe. + /// + /// + /// A message assembled up front runs whatever it interpolates on every passing + /// call, and an automation call in there can fail a wait whose predicate was + /// satisfied. It also describes the state before the wait rather than when it + /// gave up. Building it here fixes both, and routes it through Safely so a + /// description that throws cannot replace the timeout it exists to explain. + /// + private static void WaitUntil(Func predicate, Func timeoutMessage) { if (WaitFor( () => predicate() ? new object() : null, @@ -1021,7 +1014,9 @@ private static void WaitUntil(Func predicate, string timeoutMessage) return; } - throw Expired(timeoutMessage, lastError); + throw Expired( + Safely(timeoutMessage, "The wait expired and could not be described"), + lastError); } ///