From 41f89e77b67e6b66cb017be4e00235a0a9429ca7 Mon Sep 17 00:00:00 2001 From: Kai Ninomiya Date: Thu, 2 Nov 2023 17:52:43 -0700 Subject: [PATCH] Fold tools/presubmit into tools/validate (#3133) --- Gruntfile.js | 5 -- src/common/internal/test_group.ts | 43 ++++++++++++++--- src/common/tools/crawl.ts | 4 +- src/common/tools/presubmit.ts | 19 -------- src/common/tools/validate.ts | 1 + src/unittests/test_group.spec.ts | 80 +++++++++++++++++++++++++++---- tools/presubmit | 4 -- 7 files changed, 111 insertions(+), 45 deletions(-) delete mode 100644 src/common/tools/presubmit.ts delete mode 100755 tools/presubmit diff --git a/Gruntfile.js b/Gruntfile.js index 35463c091401..cf2207fcffb8 100644 --- a/Gruntfile.js +++ b/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', diff --git a/src/common/internal/test_group.ts b/src/common/internal/test_group.ts index e6b555ce4c33..632a822ef12d 100644 --- a/src/common/internal/test_group.ts +++ b/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(fixture: FixtureClass): Test // Interfaces for running tests export interface IterableTestGroup { iterate(): Iterable; - 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( 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 implements TestGroupBuilder { return test as unknown as TestBuilderWithName; } - 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 { } /** 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 { 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(); 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 { 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); } diff --git a/src/common/tools/crawl.ts b/src/common/tools/crawl.ts index 9e060f485903..50340dd68b14 100644 --- a/src/common/tools/crawl.ts +++ b/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 { - for (const suite of ['unittests', 'webgpu']) { - const loader = new DefaultTestFileLoader(); - const filterQuery = parseQuery(`${suite}:*`); - const testcases = await loader.loadCases(filterQuery); - for (const testcase of testcases) { - const name = testcase.query.toString(); - const maxLength = 375; - assert( - name.length <= maxLength, - `Testcase ${name} is too long. Max length is ${maxLength} characters. Please shorten names or reduce parameters.` - ); - } - } -})(); diff --git a/src/common/tools/validate.ts b/src/common/tools/validate.ts index 6c3375e6202f..164ee3259a3b 100644 --- a/src/common/tools/validate.ts +++ b/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/ diff --git a/src/unittests/test_group.spec.ts b/src/unittests/test_group.spec.ts index 71ffbb85faf8..aca8d298e6b9 100644 --- a/src/unittests/test_group.spec.ts +++ b/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 }) }]); diff --git a/tools/presubmit b/tools/presubmit deleted file mode 100755 index a2a3b7869012..000000000000 --- a/tools/presubmit +++ /dev/null @@ -1,4 +0,0 @@ -#!/usr/bin/env node - -require('../src/common/tools/setup-ts-in-node.js'); -require('../src/common/tools/presubmit.ts');