Catch exceptions in expectValidationError (gpuweb#416)

* Catch exceptions in expectValidationError

And fix unhandled rejection in testMapAsyncCall.

Was previously caught in WPT (due to window.onunhandledrejection) but
not in standalone.

* Remove unnecessary asynchronicity

* Add note about why expectValidationError is the way it is

src/common/framework/fixture.ts

@@ -42,10 +42,12 @@ export class Fixture {
 
     // Loop to exhaust the eventualExpectations in case they chain off each other.
     while (this.eventualExpectations.length) {
-      const previousExpectations = this.eventualExpectations;
-      this.eventualExpectations = [];
-
-      await Promise.all(previousExpectations);
+      const p = this.eventualExpectations.shift()!;
+      try {
+        await p;
+      } catch (ex) {
+        this.rec.threw(ex);
+      }
     }
   }
 

src/webgpu/api/validation/buffer/mapping.spec.ts

@@ -26,7 +26,7 @@ TODO: review existing tests and merge with this plan:
 
 import { pbool, poptions, params } from '../../../../common/framework/params_builder.js';
 import { makeTestGroup } from '../../../../common/framework/test_group.js';
-import { unreachable } from '../../../../common/framework/util/util.js';
+import { assert, unreachable } from '../../../../common/framework/util/util.js';
 import { kBufferUsages } from '../../../capability_info.js';
 import { GPUConst } from '../../../constants.js';
 import { ValidationTest } from '../validation_test.js';
@@ -43,13 +43,17 @@ class F extends ValidationTest {
     if (success) {
       const p = buffer.mapAsync(mode, offset, size);
       await p;
-      this.shouldResolve(p);
     } else {
-      this.expectValidationError(async () => {
-        const p = buffer.mapAsync(mode, offset, size);
-        await p;
-        this.shouldReject(rejectName!, p);
+      let p: Promise<void>;
+      this.expectValidationError(() => {
+        p = buffer.mapAsync(mode, offset, size);
       });
+      try {
+        await p!;
+        assert(rejectName === null, 'mapAsync unexpectedly passed');
+      } catch (ex) {
+        assert(rejectName === ex.name, `mapAsync rejected unexpectedly with: ${ex}`);
+      }
     }
   }
 

src/webgpu/api/validation/validation_test.ts

@@ -1,3 +1,4 @@
+import { assert } from '../../../common/framework/util/util.js';
 import { BindableResource } from '../../capability_info.js';
 import { GPUTest } from '../../gpu_test.js';
 
@@ -280,26 +281,45 @@ export class ValidationTest extends GPUTest {
     }
   }
 
-  expectValidationError(fn: Function, shouldError: boolean = true): void {
+  /**
+   * Expect a validation error inside the callback.
+   *
+   * Tests should always do just one WebGPU call in the callback, to make sure that's what's tested.
+   */
+  // Note: A return value is not allowed for the callback function. This is to avoid confusion
+  // about what the actual behavior would be. We could either:
+  //   - Make expectValidationError async, and have it await on fn(). This causes an async split
+  //     between pushErrorScope and popErrorScope, so if the caller doesn't `await` on
+  //     expectValidationError (either accidentally or because it doesn't care to do so), then
+  //     other test code will be (nondeterministically) caught by the error scope.
+  //   - Make expectValidationError NOT await fn(), but just execute its first block (until the
+  //     first await) and return the return value (a Promise). This would be confusing because it
+  //     would look like the error scope includes the whole async function, but doesn't.
+  expectValidationError(fn: () => void, shouldError: boolean = true): void {
     // If no error is expected, we let the scope surrounding the test catch it.
-    if (shouldError === false) {
-      fn();
-      return;
+    if (shouldError) {
+      this.device.pushErrorScope('validation');
     }
 
-    this.device.pushErrorScope('validation');
-    fn();
-    const promise = this.device.popErrorScope();
+    const returnValue = fn() as unknown;
+    assert(
+      returnValue === undefined,
+      'expectValidationError callback should not return a value (or be async)'
+    );
 
-    this.eventualAsyncExpectation(async niceStack => {
-      const gpuValidationError = await promise;
-      if (!gpuValidationError) {
-        niceStack.message = 'Validation error was expected.';
-        this.rec.validationFailed(niceStack);
-      } else if (gpuValidationError instanceof GPUValidationError) {
-        niceStack.message = `Captured validation error - ${gpuValidationError.message}`;
-        this.rec.debug(niceStack);
-      }
-    });
+    if (shouldError) {
+      const promise = this.device.popErrorScope();
+
+      this.eventualAsyncExpectation(async niceStack => {
+        const gpuValidationError = await promise;
+        if (!gpuValidationError) {
+          niceStack.message = 'Validation error was expected.';
+          this.rec.validationFailed(niceStack);
+        } else if (gpuValidationError instanceof GPUValidationError) {
+          niceStack.message = `Captured validation error - ${gpuValidationError.message}`;
+          this.rec.debug(niceStack);
+        }
+      });
+    }
   }
 }