Fold tools/presubmit into tools/validate (#3133)

Gruntfile.js

@@ -114,10 +114,6 @@ module.exports = function (grunt) {
         cmd: 'node',
         args: ['node_modules/eslint/bin/eslint', 'src/**/*.ts', '--max-warnings=0'],
       },
-      presubmit: {
-        cmd: 'node',
-        args: ['tools/presubmit'],
-      },
       fix: {
         cmd: 'node',
         args: ['node_modules/eslint/bin/eslint', 'src/**/*.ts', '--fix'],
@@ -210,7 +206,6 @@ module.exports = function (grunt) {
     'run:build-out-node',
     'build-done-message',
     'ts:check',
-    'run:presubmit',
     'run:unittest',
     'run:lint',
     'run:tsdoc-treatWarningsAsErrors',

src/common/internal/test_group.ts

@@ -19,7 +19,12 @@ import { Expectation } from '../internal/logging/result.js';
 import { TestCaseRecorder } from '../internal/logging/test_case_recorder.js';
 import { extractPublicParams, Merged, mergeParams } from '../internal/params_utils.js';
 import { compareQueries, Ordering } from '../internal/query/compare.js';
-import { TestQuerySingleCase, TestQueryWithExpectation } from '../internal/query/query.js';
+import {
+  TestQueryMultiFile,
+  TestQueryMultiTest,
+  TestQuerySingleCase,
+  TestQueryWithExpectation,
+} from '../internal/query/query.js';
 import { kPathSeparator } from '../internal/query/separators.js';
 import {
   stringifyPublicParams,
@@ -63,7 +68,7 @@ export function makeTestGroup<F extends Fixture>(fixture: FixtureClass<F>): Test
 // Interfaces for running tests
 export interface IterableTestGroup {
   iterate(): Iterable<IterableTest>;
-  validate(): void;
+  validate(fileQuery: TestQueryMultiFile): void;
   /** Returns the file-relative test paths of tests which have >0 cases. */
   collectNonEmptyTests(): { testPath: string[] }[];
 }
@@ -80,6 +85,9 @@ export function makeTestGroupForUnitTesting<F extends Fixture>(
   return new TestGroup(fixture);
 }
 
+/** The maximum allowed length of a test query string. Checked by tools/validate. */
+export const kQueryMaxLength = 375;
+
 /** Parameter name for batch number (see also TestBuilder.batch). */
 const kBatchParamName = 'batch__';
 
@@ -130,9 +138,14 @@ export class TestGroup<F extends Fixture> implements TestGroupBuilder<F> {
     return test as unknown as TestBuilderWithName<F>;
   }
 
-  validate(): void {
+  validate(fileQuery: TestQueryMultiFile): void {
     for (const test of this.tests) {
-      test.validate();
+      const testQuery = new TestQueryMultiTest(
+        fileQuery.suite,
+        fileQuery.filePathParts,
+        test.testPath
+      );
+      test.validate(testQuery);
     }
   }
 
@@ -287,7 +300,7 @@ class TestBuilder<S extends SubcaseBatchState, F extends Fixture> {
   }
 
   /** Perform various validation/"lint" chenks. */
-  validate(): void {
+  validate(testQuery: TestQueryMultiTest): void {
     const testPathString = this.testPath.join(kPathSeparator);
     assert(this.testFn !== undefined, () => {
       let s = `Test is missing .fn(): ${testPathString}`;
@@ -297,12 +310,30 @@ class TestBuilder<S extends SubcaseBatchState, F extends Fixture> {
       return s;
     });
 
+    assert(
+      testQuery.toString().length <= kQueryMaxLength,
+      () =>
+        `Test query ${testQuery} is too long. Max length is ${kQueryMaxLength} characters. Please shorten names or reduce parameters.`
+    );
+
     if (this.testCases === undefined) {
       return;
     }
 
     const seen = new Set<string>();
     for (const [caseParams, subcases] of builderIterateCasesWithSubcases(this.testCases, null)) {
+      const caseQuery = new TestQuerySingleCase(
+        testQuery.suite,
+        testQuery.filePathParts,
+        testQuery.testPathParts,
+        caseParams
+      ).toString();
+      assert(
+        caseQuery.length <= kQueryMaxLength,
+        () =>
+          `Case query ${caseQuery} is too long. Max length is ${kQueryMaxLength} characters. Please shorten names or reduce parameters.`
+      );
+
       for (const subcaseParams of subcases ?? [{}]) {
         const params = mergeParams(caseParams, subcaseParams);
         assert(this.batchSize === 0 || !(kBatchParamName in params));
@@ -319,7 +350,7 @@ class TestBuilder<S extends SubcaseBatchState, F extends Fixture> {
         const testcaseStringUnique = stringifyPublicParamsUniquely(params);
         assert(
           !seen.has(testcaseStringUnique),
-          `Duplicate public test case params for test ${testPathString}: ${testcaseString}`
+          `Duplicate public test case+subcase params for test ${testPathString}: ${testcaseString}`
         );
         seen.add(testcaseStringUnique);
       }

src/common/tools/crawl.ts

@@ -7,7 +7,7 @@ import * as path from 'path';
 
 import { loadMetadataForSuite } from '../framework/metadata.js';
 import { SpecFile } from '../internal/file_loader.js';
-import { TestQueryMultiCase } from '../internal/query/query.js';
+import { TestQueryMultiCase, TestQueryMultiFile } from '../internal/query/query.js';
 import { validQueryPart } from '../internal/query/validQueryPart.js';
 import { TestSuiteListingEntry, TestSuiteListing } from '../internal/test_suite_listing.js';
 import { assert, unreachable } from '../util/util.js';
@@ -83,7 +83,7 @@ export async function crawl(suiteDir: string, validate: boolean): Promise<TestSu
         assert(mod.description !== undefined, 'Test spec file missing description: ' + filename);
         assert(mod.g !== undefined, 'Test spec file missing TestGroup definition: ' + filename);
 
-        mod.g.validate();
+        mod.g.validate(new TestQueryMultiFile(suite, pathSegments));
 
         for (const { testPath } of mod.g.collectNonEmptyTests()) {
           const testQuery = new TestQueryMultiCase(suite, pathSegments, testPath, {}).toString();

src/common/tools/validate.ts

@@ -11,6 +11,7 @@ For each suite in SUITE_DIRS, validate some properties about the file:
   - Has a test function (or is marked unimplemented)
   - Has no duplicate cases
   - Configures batching correctly, if used
+- That each case query is not too long
 
 Example:
   tools/validate src/unittests/ src/webgpu/

src/unittests/test_group.spec.ts

@@ -5,7 +5,8 @@ Unit tests for TestGroup.
 
 import { Fixture } from '../common/framework/fixture.js';
 import { makeTestGroup } from '../common/framework/test_group.js';
-import { makeTestGroupForUnitTesting } from '../common/internal/test_group.js';
+import { TestQueryMultiFile } from '../common/internal/query/query.js';
+import { kQueryMaxLength, makeTestGroupForUnitTesting } from '../common/internal/test_group.js';
 import { assert } from '../common/util/util.js';
 
 import { TestGroupTest } from './test_group_test.js';
@@ -89,7 +90,7 @@ g.test('no_fn').fn(t => {
   g.test('missing');
 
   t.shouldThrow('Error', () => {
-    g.validate();
+    g.validate(new TestQueryMultiFile('s', ['f']));
   });
 });
 
@@ -108,13 +109,13 @@ g.test('duplicate_test_params,none').fn(() => {
     g.test('abc')
       .paramsSimple([])
       .fn(() => {});
-    g.validate();
+    g.validate(new TestQueryMultiFile('s', ['f']));
   }
 
   {
     const g = makeTestGroupForUnitTesting(UnitTest);
     g.test('abc').fn(() => {});
-    g.validate();
+    g.validate(new TestQueryMultiFile('s', ['f']));
   }
 
   {
@@ -124,7 +125,7 @@ g.test('duplicate_test_params,none').fn(() => {
         { a: 1 }, //
       ])
       .fn(() => {});
-    g.validate();
+    g.validate(new TestQueryMultiFile('s', ['f']));
   }
 });
 
@@ -137,7 +138,7 @@ g.test('duplicate_test_params,basic').fn(t => {
         { a: 1 }, //
         { a: 1 },
       ]);
-      g.validate();
+      g.validate(new TestQueryMultiFile('s', ['f']));
     });
   }
   {
@@ -151,7 +152,7 @@ g.test('duplicate_test_params,basic').fn(t => {
       )
       .fn(() => {});
     t.shouldThrow('Error', () => {
-      g.validate();
+      g.validate(new TestQueryMultiFile('s', ['f']));
     });
   }
   {
@@ -163,7 +164,7 @@ g.test('duplicate_test_params,basic').fn(t => {
       ])
       .fn(() => {});
     t.shouldThrow('Error', () => {
-      g.validate();
+      g.validate(new TestQueryMultiFile('s', ['f']));
     });
   }
 });
@@ -190,7 +191,7 @@ g.test('duplicate_test_params,with_different_private_params').fn(t => {
       )
       .fn(() => {});
     t.shouldThrow('Error', () => {
-      g.validate();
+      g.validate(new TestQueryMultiFile('s', ['f']));
     });
   }
 });
@@ -211,6 +212,67 @@ g.test('invalid_test_name').fn(t => {
   }
 });
 
+g.test('long_test_query,long_test_name').fn(t => {
+  const g = makeTestGroupForUnitTesting(UnitTest);
+
+  const long = Array(kQueryMaxLength - 5).join('a');
+
+  const fileQuery = new TestQueryMultiFile('s', ['f']);
+  g.test(long).unimplemented();
+  g.validate(fileQuery);
+
+  g.test(long + 'a').unimplemented();
+  t.shouldThrow(
+    'Error',
+    () => {
+      g.validate(fileQuery);
+    },
+    { message: long }
+  );
+});
+
+g.test('long_case_query,long_test_name').fn(t => {
+  const g = makeTestGroupForUnitTesting(UnitTest);
+
+  const long = Array(kQueryMaxLength - 5).join('a');
+
+  const fileQuery = new TestQueryMultiFile('s', ['f']);
+  g.test(long).fn(() => {});
+  g.validate(fileQuery);
+
+  g.test(long + 'a').fn(() => {});
+  t.shouldThrow(
+    'Error',
+    () => {
+      g.validate(fileQuery);
+    },
+    { message: long }
+  );
+});
+
+g.test('long_case_query,long_case_name').fn(t => {
+  const g = makeTestGroupForUnitTesting(UnitTest);
+
+  const long = Array(kQueryMaxLength - 9).join('a');
+
+  const fileQuery = new TestQueryMultiFile('s', ['f']);
+  g.test('t')
+    .paramsSimple([{ x: long }])
+    .fn(() => {});
+  g.validate(fileQuery);
+
+  g.test('u')
+    .paramsSimple([{ x: long + 'a' }])
+    .fn(() => {});
+  t.shouldThrow(
+    'Error',
+    () => {
+      g.validate(fileQuery);
+    },
+    { message: long }
+  );
+});
+
 g.test('param_value,valid').fn(() => {
   const g = makeTestGroup(UnitTest);
   g.test('a').paramsSimple([{ x: JSON.stringify({ a: 1, b: 2 }) }]);