fix(cli): count a test's blocking rate in sessions, not retry attempts

src/Xping.Cli/Report/Indexes/SessionOutcomes.cs

@@ -39,22 +39,6 @@ public static (int Tests, int Failures) Tally(TestSession session)
         return (tests, failures);
     }
 
-    /// <summary>
-    /// Returns whether a session ended with at least one test failing on its final attempt.
-    /// </summary>
-    /// <param name="session">The session to inspect.</param>
-    /// <returns><see langword="true"/> when the session ended red.</returns>
-    public static bool HasFinalFailure(TestSession session)
-    {
-        foreach (var outcome in FinalOutcomes(session).Values)
-        {
-            if (outcome.Outcome.IsFailure())
-                return true;
-        }
-
-        return false;
-    }
-
     /// <summary>
     /// Reduces a session to one outcome per test, taken from its last attempt.
     /// </summary>

src/Xping.Cli/Report/Indexes/TestIndex.cs

@@ -43,7 +43,6 @@ internal sealed class TestIndex
     private readonly Dictionary<string, int> _sessionsRunIn;
     private readonly Dictionary<string, TestReference> _references;
     private readonly Dictionary<Guid, int> _sessionPositions;
-    private readonly HashSet<Guid> _sessionsWithFinalFailures;
 
     private TestIndex(
         AnalysisWindow window,
@@ -52,7 +51,6 @@ private TestIndex(
         Dictionary<string, int> sessionsRunIn,
         Dictionary<string, TestReference> references,
         Dictionary<Guid, int> sessionPositions,
-        HashSet<Guid> sessionsWithFinalFailures,
         IReadOnlyList<string> fingerprints)
     {
         Window = window;
@@ -61,7 +59,6 @@ private TestIndex(
         _sessionsRunIn = sessionsRunIn;
         _references = references;
         _sessionPositions = sessionPositions;
-        _sessionsWithFinalFailures = sessionsWithFinalFailures;
         Fingerprints = fingerprints;
     }
 
@@ -201,30 +198,61 @@ public double RunFrequencyOf(string fingerprint)
     }
 
     /// <summary>
-    /// Gets the fraction of a test's failures that landed in a session that ended up failing.
+    /// Gets the fraction of the sessions a test failed in that it ended red.
     /// </summary>
     /// <param name="fingerprint">The test to measure.</param>
     /// <returns>A value in [0,1]; zero when the test never failed.</returns>
     /// <remarks>
+    /// <para>
     /// Separates a test that fails and blocks the build from one whose failures are always masked by
     /// a retry. Both are worth fixing; only the first is stopping anyone today.
+    /// </para>
+    /// <para>
+    /// Both counts are sessions, for the reason <see cref="RunFrequencyOf"/> gives: a retried run
+    /// records an execution per attempt, so counting attempts put every masked failure in the
+    /// denominator and none in the numerator, and a test that failed four times in one green session
+    /// and once in a red one scored 0.20 where the occasions say 0.50. The bias ran hardest against
+    /// the tests the retry findings report, and it made this term disagree about its unit with the
+    /// one immediately above it in the same scorer.
+    /// </para>
+    /// <para>
+    /// Blocking means <em>this</em> test ended the session red, read off its deciding attempt via
+    /// <see cref="RunsOf"/>. Asking instead whether the session ended red — as this did — credited a
+    /// test whose every failure was masked with blocking the build whenever some other test failed
+    /// finally in the same session, which is the opposite of the separation above.
+    /// </para>
     /// </remarks>
     public double BlockingRateOf(string fingerprint)
     {
-        int failures = 0;
-        int blocking = 0;
+        int failedSessions = 0;
+        int lastFailedSession = -1;
 
+        // Executions arrive grouped by session, so a session is new to the tally only when the last
+        // failure counted came from a different one. The same walk Build takes, and for the same
+        // reason: a retried run must not count as several occasions.
         foreach (ExecutionRef reference in ExecutionsOf(fingerprint))
         {
-            if (!reference.Failed)
+            if (!reference.Failed || reference.SessionIndex == lastFailedSession)
                 continue;
 
-            failures++;
-            if (_sessionsWithFinalFailures.Contains(reference.Session.SessionId))
+            failedSessions++;
+            lastFailedSession = reference.SessionIndex;
+        }
+
+        if (failedSessions == 0)
+            return 0;
+
+        int blocking = 0;
+
+        foreach (ExecutionRef run in RunsOf(fingerprint))
+        {
+            if (run.Failed)
                 blocking++;
         }
 
-        return failures == 0 ? 0 : (double)blocking / failures;
+        // A run that failed is a session the test failed in, so the numerator cannot outrun the
+        // denominator and the ratio needs no clamping.
+        return (double)blocking / failedSessions;
     }
 
     /// <summary>
@@ -313,7 +341,6 @@ public static TestIndex Build(AnalysisWindow window)
         var sessionsRunIn = new Dictionary<string, int>(StringComparer.Ordinal);
         var references = new Dictionary<string, TestReference>(StringComparer.Ordinal);
         var sessionPositions = new Dictionary<Guid, int>();
-        var sessionsWithFinalFailures = new HashSet<Guid>();
 
         for (int position = 0; position < window.Sessions.Count; position++)
         {
@@ -365,9 +392,6 @@ public static TestIndex Build(AnalysisWindow window)
                 if (!references.ContainsKey(fingerprint))
                     references[fingerprint] = ToReference(execution);
             }
-
-            if (SessionOutcomes.HasFinalFailure(session))
-                sessionsWithFinalFailures.Add(session.SessionId);
         }
 
         var fingerprints = byFingerprint.Keys.OrderBy(f => f, StringComparer.Ordinal).ToList();
@@ -379,7 +403,6 @@ public static TestIndex Build(AnalysisWindow window)
             sessionsRunIn,
             references,
             sessionPositions,
-            sessionsWithFinalFailures,
             fingerprints);
     }
 

tests/Xping.Cli.Tests/Report/TestIndexTests.cs

@@ -175,7 +175,7 @@ public void ARunAgreesWithSessionOutcomesWhenAttemptsArriveOutOfOrder()
             TestIndex.Build(TestSessionFactory.Window(session)).RunsOf(SubjectFingerprint));
 
         Assert.True(run.Failed);
-        Assert.True(SessionOutcomes.HasFinalFailure(session));
+        Assert.Equal((1, 1), SessionOutcomes.Tally(session));
     }
 
     [Fact]
@@ -196,6 +196,110 @@ public void RunsAreOrderedNewestSessionFirstLikeExecutions()
         Assert.Equal(Enumerable.Range(0, 8), runs.Select(r => r.SessionIndex));
     }
 
+    // ---------------------------------------------------------------------------------------
+    // Blocking rate
+    // ---------------------------------------------------------------------------------------
+
+    /// <summary>
+    /// Builds one session in which <see cref="Subject"/> took <paramref name="attempts"/> attempts
+    /// and ended the way <paramref name="ends"/> says.
+    /// </summary>
+    /// <param name="ordinal">The session's position; 0 is the oldest.</param>
+    /// <param name="attempts">How many attempts the subject took.</param>
+    /// <param name="ends">The outcome of the deciding attempt; every earlier one failed.</param>
+    /// <param name="alsoFailing">A second test that ends the session red on its only attempt.</param>
+    private static TestSession Attempted(
+        int ordinal,
+        int attempts,
+        TestOutcome ends,
+        bool alsoFailing = false)
+    {
+        var executions = new List<TestExecution>();
+
+        for (int attempt = 1; attempt <= attempts; attempt++)
+        {
+            bool deciding = attempt == attempts;
+
+            executions.Add(TestSessionFactory.Execution(
+                Subject,
+                outcome: deciding ? ends : TestOutcome.Failed,
+                attempt: attempt,
+                passedOnRetry: deciding && ends == TestOutcome.Passed && attempts > 1,
+                maxRetries: attempts - 1,
+                errorMessage: deciding && ends == TestOutcome.Passed ? null : "boom"));
+        }
+
+        executions.Add(alsoFailing
+            ? TestSessionFactory.Execution("Neighbour", TestOutcome.Failed, errorMessage: "boom")
+            : TestSessionFactory.Execution("Neighbour"));
+
+        return TestSessionFactory.Session(ordinal, executions);
+    }
+
+    [Fact]
+    public void BlockingRateCountsSessionsRatherThanAttempts()
+    {
+        // The bug this pins: four failed attempts in one session that ended green and one failure in
+        // a session that ended red is five failed executions of which one blocked — 0.20 — where the
+        // occasions say one of two, 0.50. Counting attempts made a test look less blocking the more
+        // it retried, which is the opposite of the truth and worst for the tests the retry findings
+        // already report.
+        TestIndex index = TestIndex.Build(TestSessionFactory.Window(
+            Attempted(0, attempts: 5, ends: TestOutcome.Passed),
+            Attempted(1, attempts: 1, ends: TestOutcome.Failed)));
+
+        Assert.Equal(0.50, index.BlockingRateOf(SubjectFingerprint), 3);
+    }
+
+    [Fact]
+    public void AMaskedFailureIsNotBlockingEvenWhenAnotherTestFailedTheSession()
+    {
+        // Blocking has to mean this test ended the session red. Asking whether the session ended red
+        // credited a test whose every failure was masked with blocking the build whenever a
+        // neighbour failed finally in the same session.
+        TestIndex index = TestIndex.Build(TestSessionFactory.Window(
+            Attempted(0, attempts: 3, ends: TestOutcome.Passed, alsoFailing: true),
+            Attempted(1, attempts: 2, ends: TestOutcome.Passed, alsoFailing: true)));
+
+        Assert.Equal(0, index.BlockingRateOf(SubjectFingerprint));
+    }
+
+    [Fact]
+    public void ATestThatEndsEverySessionRedBlocksEveryTimeItFails()
+    {
+        TestIndex index = TestIndex.Build(TestSessionFactory.Window(
+            Attempted(0, attempts: 1, ends: TestOutcome.Failed),
+            Attempted(1, attempts: 3, ends: TestOutcome.Failed)));
+
+        Assert.Equal(1.0, index.BlockingRateOf(SubjectFingerprint));
+    }
+
+    [Fact]
+    public void ATimeoutCountsAsAFailureAndAsABlock()
+    {
+        // Failure is TestOutcome.Failed or TestOutcome.Timeout, and both counts read it the same way.
+        TestIndex index = TestIndex.Build(TestSessionFactory.Window(
+            Attempted(0, attempts: 1, ends: TestOutcome.Timeout)));
+
+        Assert.Equal(1.0, index.BlockingRateOf(SubjectFingerprint));
+    }
+
+    [Fact]
+    public void BlockingRateIsZeroForATestThatNeverFailed()
+    {
+        TestIndex index = TestIndex.Build(Window(total: 6, presentIn: 6, attempts: 1));
+
+        Assert.Equal(0, index.BlockingRateOf(SubjectFingerprint));
+    }
+
+    [Fact]
+    public void BlockingRateIsZeroForAFingerprintTheWindowNeverSaw()
+    {
+        TestIndex index = TestIndex.Build(Window(total: 6, presentIn: 6, attempts: 2));
+
+        Assert.Equal(0, index.BlockingRateOf("fp-NeverRan"));
+    }
+
     // ---------------------------------------------------------------------------------------
     // Recency
     // ---------------------------------------------------------------------------------------