From 10571659947f5f644f066a6740521df35184eb92 Mon Sep 17 00:00:00 2001 From: Kai Ninomiya Date: Tue, 22 Dec 2020 11:51:39 -0800 Subject: [PATCH] Catch exceptions in expectValidationError (#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 | 10 ++-- .../api/validation/buffer/mapping.spec.ts | 16 +++--- src/webgpu/api/validation/validation_test.ts | 54 +++++++++++++------ 3 files changed, 53 insertions(+), 27 deletions(-) diff --git a/src/common/framework/fixture.ts b/src/common/framework/fixture.ts index ee6a4e51a7..39bd192bfc 100644 --- a/src/common/framework/fixture.ts +++ b/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); + } } } diff --git a/src/webgpu/api/validation/buffer/mapping.spec.ts b/src/webgpu/api/validation/buffer/mapping.spec.ts index 8c8c178765..255767280d 100644 --- a/src/webgpu/api/validation/buffer/mapping.spec.ts +++ b/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; + 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}`); + } } } diff --git a/src/webgpu/api/validation/validation_test.ts b/src/webgpu/api/validation/validation_test.ts index 0d13e871a4..b8a25d4963 100644 --- a/src/webgpu/api/validation/validation_test.ts +++ b/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); + } + }); + } } }