Make Pass > Skip (gpuweb#3054)

As it is, the test recorder only keeps the "highest" status. It
was Pass = 0, Skip = 1, that means if 1000 subcases passes but just
one was skipped the entire case was marked as skip.

Making Pass > Skip means that as long as one subcase runs
the test will be marked as Pass unless there is a higher status
subcase

src/common/internal/logging/result.ts

@@ -3,7 +3,7 @@ import { LogMessageWithStack } from './log_message.js';
 // MAINTENANCE_TODO: Add warn expectations
 export type Expectation = 'pass' | 'skip' | 'fail';
 
-export type Status = 'running' | 'warn' | Expectation;
+export type Status = 'notrun' | 'running' | 'warn' | Expectation;
 
 export interface TestCaseResult {
   status: Status;

src/common/internal/logging/test_case_recorder.ts

@@ -3,27 +3,43 @@ import { globalTestConfig } from '../../framework/test_config.js';
 import { now, assert } from '../../util/util.js';
 
 import { LogMessageWithStack } from './log_message.js';
-import { Expectation, LiveTestCaseResult } from './result.js';
+import { Expectation, LiveTestCaseResult, Status } from './result.js';
 
 enum LogSeverity {
-  Pass = 0,
+  NotRun = 0,
   Skip = 1,
-  Warn = 2,
-  ExpectFailed = 3,
-  ValidationFailed = 4,
-  ThrewException = 5,
+  Pass = 2,
+  Warn = 3,
+  ExpectFailed = 4,
+  ValidationFailed = 5,
+  ThrewException = 6,
 }
 
 const kMaxLogStacks = 2;
 const kMinSeverityForStack = LogSeverity.Warn;
 
+function logSeverityToString(status: LogSeverity): Status {
+  switch (status) {
+    case LogSeverity.NotRun:
+      return 'notrun';
+    case LogSeverity.Pass:
+      return 'pass';
+    case LogSeverity.Skip:
+      return 'skip';
+    case LogSeverity.Warn:
+      return 'warn';
+    default:
+      return 'fail'; // Everything else is an error
+  }
+}
+
 /** Holds onto a LiveTestCaseResult owned by the Logger, and writes the results into it. */
 export class TestCaseRecorder {
   readonly result: LiveTestCaseResult;
   public nonskippedSubcaseCount: number = 0;
   private inSubCase: boolean = false;
-  private subCaseStatus = LogSeverity.Pass;
-  private finalCaseStatus = LogSeverity.Pass;
+  private subCaseStatus = LogSeverity.NotRun;
+  private finalCaseStatus = LogSeverity.NotRun;
   private hideStacksBelowSeverity = kMinSeverityForStack;
   private startTime = -1;
   private logs: LogMessageWithStack[] = [];
@@ -56,20 +72,13 @@ export class TestCaseRecorder {
     }
 
     // Convert numeric enum back to string (but expose 'exception' as 'fail')
-    this.result.status =
-      this.finalCaseStatus === LogSeverity.Pass
-        ? 'pass'
-        : this.finalCaseStatus === LogSeverity.Skip
-        ? 'skip'
-        : this.finalCaseStatus === LogSeverity.Warn
-        ? 'warn'
-        : 'fail'; // Everything else is an error
+    this.result.status = logSeverityToString(this.finalCaseStatus);
 
     this.result.logs = this.logs;
   }
 
   beginSubCase() {
-    this.subCaseStatus = LogSeverity.Pass;
+    this.subCaseStatus = LogSeverity.NotRun;
     this.inSubCase = true;
   }
 
@@ -87,9 +96,7 @@ export class TestCaseRecorder {
       }
     } finally {
       this.inSubCase = false;
-      if (this.subCaseStatus > this.finalCaseStatus) {
-        this.finalCaseStatus = this.subCaseStatus;
-      }
+      this.finalCaseStatus = Math.max(this.finalCaseStatus, this.subCaseStatus);
     }
   }
 
@@ -103,7 +110,8 @@ export class TestCaseRecorder {
   }
 
   info(ex: Error): void {
-    this.logImpl(LogSeverity.Pass, 'INFO', ex);
+    // We need this to use the lowest LogSeverity so it doesn't override the current severity for this test case.
+    this.logImpl(LogSeverity.NotRun, 'INFO', ex);
   }
 
   skipped(ex: SkipTestCase): void {
@@ -122,6 +130,14 @@ export class TestCaseRecorder {
     this.logImpl(LogSeverity.ValidationFailed, 'VALIDATION FAILED', ex);
   }
 
+  passed(): void {
+    if (this.inSubCase) {
+      this.subCaseStatus = Math.max(this.subCaseStatus, LogSeverity.Pass);
+    } else {
+      this.finalCaseStatus = Math.max(this.finalCaseStatus, LogSeverity.Pass);
+    }
+  }
+
   threw(ex: unknown): void {
     if (ex instanceof SkipTestCase) {
       this.skipped(ex);
@@ -137,9 +153,9 @@ export class TestCaseRecorder {
 
     // Final case status should be the "worst" of all log entries.
     if (this.inSubCase) {
-      if (level > this.subCaseStatus) this.subCaseStatus = level;
+      this.subCaseStatus = Math.max(this.subCaseStatus, level);
     } else {
-      if (level > this.finalCaseStatus) this.finalCaseStatus = level;
+      this.finalCaseStatus = Math.max(this.finalCaseStatus, level);
     }
 
     // setFirstLineOnly for all logs except `kMaxLogStacks` stacks at the highest severity

src/common/internal/test_group.ts

@@ -491,6 +491,7 @@ class RunCaseSpecific implements RunCase {
       try {
         await inst.init();
         await this.fn(inst as Fixture & { params: {} });
+        rec.passed();
       } finally {
         // Runs as long as constructor succeeded, even if initialization or the test failed.
         await inst.finalize();

src/unittests/logger.spec.ts

@@ -36,6 +36,18 @@ g.test('empty').fn(t => {
   t.expect(res.status === 'running');
   rec.finish();
 
+  t.expect(res.status === 'notrun');
+  t.expect(res.timems >= 0);
+});
+
+g.test('passed').fn(t => {
+  const mylog = new Logger({ overrideDebugMode: true });
+  const [rec, res] = mylog.record('one');
+
+  rec.start();
+  rec.passed();
+  rec.finish();
+
   t.expect(res.status === 'pass');
   t.expect(res.timems >= 0);
 });
@@ -59,13 +71,27 @@ g.test('skip').fn(t => {
 
   rec.start();
   rec.skipped(new SkipTestCase());
-  rec.debug(new Error('hello'));
   rec.finish();
 
   t.expect(res.status === 'skip');
   t.expect(res.timems >= 0);
 });
 
+// Tests if there's some skips and at least one pass it's pass.
+g.test('skip_pass').fn(t => {
+  const mylog = new Logger({ overrideDebugMode: true });
+  const [rec, res] = mylog.record('one');
+
+  rec.start();
+  rec.skipped(new SkipTestCase());
+  rec.debug(new Error('hello'));
+  rec.skipped(new SkipTestCase());
+  rec.finish();
+
+  t.expect(res.status === 'pass');
+  t.expect(res.timems >= 0);
+});
+
 g.test('warn').fn(t => {
   const mylog = new Logger({ overrideDebugMode: true });
   const [rec, res] = mylog.record('one');

src/unittests/test_group.spec.ts

@@ -262,6 +262,29 @@ g.test('subcases').fn(async t0 => {
   t0.expect(Array.from(result.values()).every(v => v.status === 'pass'));
 });
 
+g.test('subcases,skip')
+  .desc(
+    'If all tests are skipped then status is "skip". If at least one test passed, status is "pass"'
+  )
+  .params(u => u.combine('allSkip', [false, true]))
+  .fn(async t0 => {
+    const { allSkip } = t0.params;
+    const g = makeTestGroupForUnitTesting(UnitTest);
+    g.test('a')
+      .params(u => u.beginSubcases().combine('do', ['pass', 'skip', 'pass']))
+      .fn(t => {
+        t.skipIf(allSkip || t.params.do === 'skip');
+      });
+    const result = await t0.run(g);
+    const values = Array.from(result.values());
+    t0.expect(values.length === 1);
+    const expectedStatus = allSkip ? 'skip' : 'pass';
+    t0.expect(
+      values[0].status === expectedStatus,
+      `expect: ${values[0].status} === ${expectedStatus}}, allSkip: ${allSkip}`
+    );
+  });
+
 g.test('exceptions')
   .params(u =>
     u