Fix expectCompileResult and variable_and_const tests (#652)

* Fix expectCompileResult and variable_and_const tests * split subcases
gpuweb · Jul 22, 2021 · 524aa50 · 524aa50
1 parent 2f03d04
commit 524aa50
Show file tree

Hide file tree

Showing 3 changed files with 96 additions and 127 deletions.
diff --git a/src/common/util/util.ts b/src/common/util/util.ts
@@ -7,7 +7,7 @@ import { timeout } from './timeout.js';
  * The extra data is omitted if not running the test in debug mode (`?debug=1`).
  */
 export class ErrorWithExtra extends Error {
-  readonly extra: {};
+  readonly extra: { [k: string]: unknown };
 
   /**
    * `extra` function is only called if in debug mode.

diff --git a/src/webgpu/shader/validation/shader_validation_test.ts b/src/webgpu/shader/validation/shader_validation_test.ts
@@ -1,3 +1,4 @@
+import { ErrorWithExtra } from '../../../common/util/util.js';
 import { GPUTest } from '../../gpu_test.js';
 
 /**
@@ -13,61 +14,63 @@ export class ShaderValidationTest extends GPUTest {
    * t.expectCompileResult(false, `wgsl code`); // Expect validation error with any error string
    * t.expectCompileResult('substr', `wgsl code`); // Expect validation error containing 'substr'
    * ```
+   *
+   * TODO(gpuweb/gpuweb#1813): Remove the "string" overload if there are no standard error codes.
    */
-  expectCompileResult(result: boolean | string, code: string) {
-    // If an error is expected, push an error scope to catch it.
-    // Otherwise, the test harness will catch unexpected errors.
-    if (result !== true) {
-      this.device.pushErrorScope('validation');
-    }
+  expectCompileResult(expectedResult: boolean | string, code: string) {
+    let shaderModule: GPUShaderModule;
+    this.expectGPUError(
+      'validation',
+      () => {
+        shaderModule = this.device.createShaderModule({ code });
+      },
+      expectedResult !== true
+    );
 
-    const shaderModule = this.device.createShaderModule({ code });
+    const error = new ErrorWithExtra('', () => ({ shaderModule }));
+    (async () => {
+      const compilationInfo = await shaderModule!.compilationInfo();
 
-    if (result !== true) {
-      const promise = this.device.popErrorScope();
+      // TODO: Pretty-print error messages with source context.
+      const messagesLog = compilationInfo.messages
+        .map(m => `${m.lineNum}:${m.linePos}: ${m.type}: ${m.message}`)
+        .join('\n');
+      error.extra.compilationInfo = compilationInfo;
 
-      this.eventualAsyncExpectation(async niceStack => {
-        // TODO: This is a non-compliant fallback path for Chrome, which doesn't
-        // implement .compilationInfo() yet. Remove it.
-        if (!shaderModule.compilationInfo) {
-          const gpuValidationError = await promise;
-          if (!gpuValidationError) {
-            niceStack.message = 'Compilation succeeded unexpectedly.';
-            this.rec.validationFailed(niceStack);
-          } else if (gpuValidationError instanceof GPUValidationError) {
-            if (typeof result === 'string' && gpuValidationError.message.indexOf(result) === -1) {
-              niceStack.message = `Compilation failed, but message missing expected substring \
-«${result}» - ${gpuValidationError.message}`;
-              this.rec.validationFailed(niceStack);
-            } else {
-              niceStack.message = `Compilation failed, as expected - ${gpuValidationError.message}`;
-              this.rec.debug(niceStack);
-            }
+      if (typeof expectedResult === 'string') {
+        for (const msg of compilationInfo.messages) {
+          if (msg.type === 'error' && msg.message.indexOf(expectedResult) !== -1) {
+            error.message =
+              `Found expected compilationInfo message substring «${expectedResult}».\n` +
+              messagesLog;
+            this.rec.debug(error);
+            return;
           }
-          return;
         }
 
-        if (typeof result === 'string') {
-          const info = await shaderModule.compilationInfo();
-          for (const message of info.messages) {
-            if (message.type === 'error' && message.message.indexOf(result) !== -1) {
-              niceStack.message = `Compilation failed, as expected - \
-${message.lineNum}:${message.linePos}: ${message.message}`;
-              this.rec.debug(niceStack);
-              return;
-            }
-          }
-          // Here, the expected string was not found.
+        // Here, no error message was found, but one was expected.
+        error.message = `Missing expected substring «${expectedResult}».\n` + messagesLog;
+        this.rec.validationFailed(error);
+        return;
+      }
 
-          // TODO: Pretty-print error messages, with source context.
-          const messagesLog = info.messages
-            .map(m => `${m.lineNum}:${m.linePos}: ${m.type}: ${m.message}`)
-            .join('\n');
-          niceStack.message = `Compilation failed, but no error message with expected substring \
-«${result}»\n${messagesLog}`;
-          this.rec.validationFailed(niceStack);
+      if (compilationInfo.messages.some(m => m.type === 'error')) {
+        if (expectedResult) {
+          error.message = `Unexpected compilationInfo 'error' message.\n` + messagesLog;
+          this.rec.validationFailed(error);
+        } else {
+          error.message = `Found expected compilationInfo 'error' message.\n` + messagesLog;
+          this.rec.debug(error);
+        }
+      } else {
+        if (!expectedResult) {
+          error.message = `Missing expected compilationInfo 'error' message.\n` + messagesLog;
+          this.rec.validationFailed(error);
+        } else {
+          error.message = `No compilationInfo 'error' messages, as expected.\n` + messagesLog;
+          this.rec.debug(error);
         }
-      });
-    }
+      }
+    })();
   }
 }
diff --git a/src/webgpu/shader/validation/variable_and_const.spec.ts b/src/webgpu/shader/validation/variable_and_const.spec.ts
@@ -8,60 +8,38 @@ import { ShaderValidationTest } from './shader_validation_test.js';
 
 export const g = makeTestGroup(ShaderValidationTest);
 
-const kScalarType = ['i32', 'f32', 'u32', 'bool'] as const;
-type ScalarType = 'i32' | 'f32' | 'u32' | 'bool';
-
-const kContainerTypes = [
-  undefined,
-  'vec2',
-  'vec3',
-  'vec4',
-  'mat2x2',
-  'mat2x3',
-  'mat2x4',
-  'mat3x2',
-  'mat3x3',
-  'mat3x4',
-  'mat4x2',
-  'mat4x3',
-  'mat4x4',
-  'array',
+const kTestTypes = [
+  'f32',
+  'i32',
+  'u32',
+  'bool',
+  'vec2<f32>',
+  'vec2<i32>',
+  'vec2<u32>',
+  'vec2<bool>',
+  'vec3<f32>',
+  'vec3<i32>',
+  'vec3<u32>',
+  'vec3<bool>',
+  'vec4<f32>',
+  'vec4<i32>',
+  'vec4<u32>',
+  'vec4<bool>',
+  'mat2x2<f32>',
+  'mat2x3<f32>',
+  'mat2x4<f32>',
+  'mat3x2<f32>',
+  'mat3x3<f32>',
+  'mat3x4<f32>',
+  'mat4x2<f32>',
+  'mat4x3<f32>',
+  'mat4x4<f32>',
+  // TODO(sarahM0): 12 is a random number here. find a solution to replace it.
+  'array<f32, 12>',
+  'array<i32, 12>',
+  'array<u32, 12>',
+  'array<bool, 12>',
 ] as const;
-type ContainerType =
-  | undefined
-  | 'vec2'
-  | 'vec3'
-  | 'vec4'
-  | 'mat2x2'
-  | 'mat2x3'
-  | 'mat2x4'
-  | 'mat3x2'
-  | 'mat3x3'
-  | 'mat3x4'
-  | 'mat4x2'
-  | 'mat4x3'
-  | 'mat4x4'
-  | 'array';
-
-function getType(scalarType: ScalarType, containerType: ContainerType) {
-  let type = '';
-  switch (containerType) {
-    case undefined: {
-      type = scalarType;
-      break;
-    }
-    case 'array': {
-      // TODO(sarahM0): 12 is a random number here. find a solution to replace it.
-      type = `array<${scalarType}, 12>`;
-      break;
-    }
-    default: {
-      type = `${containerType}<${scalarType}>`;
-      break;
-    }
-  }
-  return type;
-}
 
 g.test('initializer_type')
   .desc(
@@ -73,23 +51,13 @@ g.test('initializer_type')
   )
   .params(u =>
     u
-      .combine('variableOrConstant', ['var', 'const'])
-      .combine('lhsContainerType', kContainerTypes)
-      .combine('lhsScalarType', kScalarType)
-      .combine('rhsContainerType', kContainerTypes)
-      .combine('rhsScalarType', kScalarType)
+      .combine('variableOrConstant', ['var', 'let'])
+      .beginSubcases()
+      .combine('lhsType', kTestTypes)
+      .combine('rhsType', kTestTypes)
   )
   .fn(t => {
-    const {
-      variableOrConstant,
-      lhsContainerType,
-      lhsScalarType,
-      rhsContainerType,
-      rhsScalarType,
-    } = t.params;
-
-    const lhsType = getType(lhsScalarType, lhsContainerType);
-    const rhsType = getType(rhsScalarType, rhsContainerType);
+    const { variableOrConstant, lhsType, rhsType } = t.params;
 
     const code = `
       [[stage(fragment)]]
@@ -98,7 +66,7 @@ g.test('initializer_type')
       }
     `;
 
-    const expectation = lhsScalarType === rhsScalarType && lhsContainerType === rhsContainerType;
+    const expectation = lhsType === rhsType;
     t.expectCompileResult(expectation, code);
   });
 
@@ -123,17 +91,13 @@ g.test('io_shareable_type')
 
   Control case: 'private' is used to make sure when only the storage class changes, the shader
   becomes invalid and nothing else is wrong.
-  TODO: add test for: struct - struct with bool component - struct with runtime array`
-  )
-  .params(u =>
-    u
-      .combine('storageClass', ['in', 'out', 'private'])
-      .combine('containerType', kContainerTypes)
-      .combine('scalarType', kScalarType)
+  TODO: add test for structs:
+  - struct with bool component
+  - struct with runtime array`
   )
+  .params(u => u.combine('storageClass', ['in', 'out', 'private']).combine('type', kTestTypes))
   .fn(t => {
-    const { storageClass, containerType, scalarType } = t.params;
-    const type = containerType ? `${containerType}<${scalarType}>` : scalarType;
+    const { storageClass, type } = t.params;
 
     let code;
     if (`${storageClass}` === 'in') {
@@ -167,6 +131,8 @@ g.test('io_shareable_type')
       `;
     }
 
-    const expectation = storageClass === 'private' || scalarType !== 'bool';
+    const expectation =
+      storageClass === 'private' ||
+      (type.indexOf('bool') === -1 && !type.startsWith('mat') && !type.startsWith('array'));
     t.expectCompileResult(expectation, code);
   });