diff --git a/src/scripts/lint.ts b/src/scripts/lint.ts index 60074439..e9fbe7fa 100644 --- a/src/scripts/lint.ts +++ b/src/scripts/lint.ts @@ -52,8 +52,8 @@ function getLintDiffCommands(options: Options): Record { '*': 'prettier --ignore-unknown' + (options.fix ? ' --write' : ' --check'), '*.{ts,js,tsx,jsx}': 'eslint --config src/.eslintrc.js' + (options.fix ? ' --fix' : ''), - 'studies/**/*.json': - 'npm run seed_tools -- check_study' + (options.fix ? ' --fix' : ''), + 'studies/*': () => + 'npm run seed_tools -- lint studies' + (options.fix ? ' --fix' : ''), }; } diff --git a/src/seed_tools/README.md b/src/seed_tools/README.md index 340fd600..4c679dc7 100644 --- a/src/seed_tools/README.md +++ b/src/seed_tools/README.md @@ -4,88 +4,27 @@ ## Tools -### `check_study` - -Validates study files for both logic and format errors. Called automatically -during `lint` for `studies/**/*.json` files. - -##### Syntax - -```bash -npm run seed_tools -- check_study [--fix] -``` - -##### Arguments - -- ``: One or more study files that you want to check. - -##### Options - -- `--fix`: Fix format errors in-place. - -### `create_seed` - -Generates a `seed.bin` file from study files. - -##### Syntax - -```bash -npm run seed_tools -- create_seed [--mock_serial_number ] [--serial_number_path ] -``` - -##### Arguments - -- ``: The directory containing the study files. -- ``: The output file for the generated seed. - -##### Options - -- `--mock_serial_number `: Mock a serial number. If not provided, a - random number is used. -- `--serial_number_path `: The file path to write the serial number to. - ### `compare_seeds` Compares two seed binary files and displays a human-readable diff. Used for safe migration from the python seed generator to the typescript seed generator. -##### Syntax +### `create` -```bash -npm run seed_tools -- compare_seeds -``` +Generates a `seed.bin` file from study files. -##### Arguments +### `lint` -- ``: The first seed binary file to compare. -- ``: The second seed binary file to compare. +Lints study files. ### `split_seed_json` Splits a legacy `seed.json` file into individual study files. -##### Syntax - -```bash -npm run seed_tools -- split_seed_json -``` - -##### Arguments - -- ``: The path to the `seed.json` file to be split. -- ``: The directory where the individual study files will be - outputted. +## Tools help -### `validate_seed` - -Validates a seed protobuf. - -##### Syntax +Run to get available arguments and options: ```bash -npm run seed_tools -- validate_seed +npm run seed_tools -- --help ``` - -##### Arguments - -- ``: The path to the binary-serialized `seed` protobuf. diff --git a/src/seed_tools/commands/check_study.test.ts b/src/seed_tools/commands/check_study.test.ts deleted file mode 100644 index fdc9e401..00000000 --- a/src/seed_tools/commands/check_study.test.ts +++ /dev/null @@ -1,132 +0,0 @@ -// Copyright (c) 2024 The Brave Authors. All rights reserved. -// This Source Code Form is subject to the terms of the Mozilla Public -// License, v. 2.0. If a copy of the MPL was not distributed with this file, -// You can obtain one at https://mozilla.org/MPL/2.0/. - -import * as fss from 'fs'; -import { promises as fs } from 'fs'; -import * as os from 'os'; -import * as path from 'path'; -import { wsPath } from '../../base/path_utils'; -import check_study from './check_study'; - -describe('check_study command', () => { - const testDataDir = wsPath('//src/test/data'); - - let tempDir: string; - let exitMock: jest.SpyInstance; - let errorMock: jest.SpyInstance; - - beforeEach(async () => { - tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'check_study-')); - exitMock = jest.spyOn(process, 'exit').mockImplementation(); - errorMock = jest.spyOn(console, 'error').mockImplementation(); - }); - - afterEach(async () => { - await fs.rm(tempDir, { recursive: true, force: true }); - exitMock.mockRestore(); - errorMock.mockRestore(); - }); - - describe('valid studies', () => { - const validSeedsDir = path.join(testDataDir, 'valid_seeds'); - it.each(fss.readdirSync(validSeedsDir))( - 'correctly validates %s', - async (testCase) => { - const testCaseDir = path.join(validSeedsDir, testCase); - const studyFiles = fss.readdirSync(path.join(testCaseDir, 'studies')); - - for (const study of studyFiles) { - const studyFile = path.join(testCaseDir, 'studies', study); - - await check_study.parseAsync(['node', 'check_study', studyFile]); - expect(exitMock).toHaveBeenCalledTimes(0); - expect(errorMock).toHaveBeenCalledTimes(0); - } - }, - ); - }); - - describe('invalid studies', () => { - const invalidStudiesDir = path.join(testDataDir, 'invalid_studies'); - - it.each(fss.readdirSync(invalidStudiesDir))( - 'correctly validates %s', - async (testCase) => { - const testCaseDir = path.join(invalidStudiesDir, testCase); - const studyFiles = fss.readdirSync(path.join(testCaseDir, 'studies')); - - for (const study of studyFiles) { - const studyFile = path.join(testCaseDir, 'studies', study); - - await check_study.parseAsync(['node', 'check_study', studyFile]); - expect(exitMock).toHaveBeenCalledWith(1); - expect(exitMock).toHaveBeenCalledTimes(1); - exitMock.mockClear(); - - const expectedOutput = await fs.readFile( - path.join(testCaseDir, 'expected_errors.txt'), - 'utf-8', - ); - const expectedOutputLines = expectedOutput.split('\n'); - expect(expectedOutputLines.length).toBeGreaterThan(0); - for (const line of expectedOutputLines) { - expect(errorMock).toHaveBeenCalledWith( - expect.stringContaining(line), - ); - } - } - }, - ); - }); - - describe('unformatted studies', () => { - const unformattedStudiesDir = path.join(testDataDir, 'unformatted_studies'); - - it.each(fss.readdirSync(unformattedStudiesDir))( - 'correctly validates %s', - async (testCase) => { - const testCaseDir = path.join(unformattedStudiesDir, testCase); - const studyFiles = fss.readdirSync(path.join(testCaseDir, 'studies')); - - for (const study of studyFiles) { - const studyFile = path.join(testCaseDir, 'studies', study); - - await check_study.parseAsync(['node', 'check_study', studyFile]); - expect(exitMock).toHaveBeenCalledWith(1); - expect(exitMock).toHaveBeenCalledTimes(1); - exitMock.mockClear(); - - const expectedOutput = await fs.readFile( - path.join(testCaseDir, 'expected_output.txt'), - 'utf-8', - ); - expect(errorMock).toHaveBeenCalledWith( - expect.stringContaining(expectedOutput), - ); - } - }, - ); - }); - - describe('studies should be valid in invalid_seed test dir', () => { - const invalidSeedsDir = path.join(testDataDir, 'invalid_seeds'); - - it.each(fss.readdirSync(invalidSeedsDir))( - 'correctly validates %s', - async (testCase) => { - const testCaseDir = path.join(invalidSeedsDir, testCase); - const studyFiles = fss.readdirSync(path.join(testCaseDir, 'studies')); - - for (const study of studyFiles) { - const studyFile = path.join(testCaseDir, 'studies', study); - - await check_study.parseAsync(['node', 'check_study', studyFile]); - expect(exitMock).toHaveBeenCalledTimes(0); - expect(errorMock).toHaveBeenCalledTimes(0); - } - }, - ); - }); -}); diff --git a/src/seed_tools/commands/check_study.ts b/src/seed_tools/commands/check_study.ts deleted file mode 100644 index d802156e..00000000 --- a/src/seed_tools/commands/check_study.ts +++ /dev/null @@ -1,89 +0,0 @@ -// Copyright (c) 2024 The Brave Authors. All rights reserved. -// This Source Code Form is subject to the terms of the Mozilla Public -// License, v. 2.0. If a copy of the MPL was not distributed with this file, -// You can obtain one at https://mozilla.org/MPL/2.0/. - -import { Command } from '@commander-js/extra-typings'; -import { promises as fs } from 'fs'; -import DefaultMap from 'src/base/containers/default_map'; -import { type Study } from '../../proto/generated/study'; -import diffStrings from '../utils/diff_strings'; -import * as study_json_utils from '../utils/study_json_utils'; -import * as study_validation from '../utils/study_validation'; - -export default new Command('check_study') - .description('Check study files for logic and format errors') - .argument('', 'study files') - .option('--fix', 'fix format errors in-place') - .action(main); - -interface Options { - fix?: true; -} - -async function main(studyFilePaths: string[], options: Options) { - const errorsPerFile = new DefaultMap(() => []); - for (const studyFilePath of studyFilePaths) { - const readStudyFileResult = - await study_json_utils.readStudyFileReturnWithError(studyFilePath); - if (readStudyFileResult instanceof Error) { - errorsPerFile.get(studyFilePath).push(readStudyFileResult.message); - continue; - } - - const [studies, studyArrayString] = readStudyFileResult; - for (const study of studies) { - const studyErrors = study_validation.validateStudyReturnErrors( - study, - studyFilePath, - ); - if (studyErrors.length > 0) { - errorsPerFile.get(studyFilePath).push(...studyErrors); - } - } - - const formatErrors = await checkAndOptionallyFixFormat( - studyFilePath, - studies, - studyArrayString, - options, - ); - if (formatErrors.length > 0) { - errorsPerFile.get(studyFilePath).push(...formatErrors); - } - } - - for (const [studyFilePath, errors] of errorsPerFile.entries()) { - console.error(`Errors in ${studyFilePath}:\n${errors.join('\n---\n')}`); - } - - if (errorsPerFile.size > 0) { - process.exit(1); - } -} - -async function checkAndOptionallyFixFormat( - studyFilePath: string, - studies: Study[], - studyArrayString: string, - options: Options, -): Promise { - const errors: string[] = []; - const stringifiedStudyArray = study_json_utils.stringifyStudyArray(studies); - if (stringifiedStudyArray !== studyArrayString) { - if (options.fix) { - await fs.writeFile(studyFilePath, stringifiedStudyArray); - } else { - errors.push( - `Format required:\n` + - (await diffStrings( - studyArrayString, - stringifiedStudyArray, - studyFilePath, - studyFilePath + '.formatted', - )), - ); - } - } - return errors; -} diff --git a/src/seed_tools/commands/compare_seeds.ts b/src/seed_tools/commands/compare_seeds.ts index 4f2bba52..b2e84110 100644 --- a/src/seed_tools/commands/compare_seeds.ts +++ b/src/seed_tools/commands/compare_seeds.ts @@ -8,11 +8,13 @@ import { promises as fs } from 'fs'; import { VariationsSeed } from '../../proto/generated/variations_seed'; import diffStrings from '../utils/diff_strings'; -export default new Command('compare_seeds') - .description('Compare two seed.bin') - .argument('', 'seed1 file') - .argument('', 'seed2 file') - .action(main); +export default function createCommand() { + return new Command('compare_seeds') + .description('Compare two seed.bin') + .argument('', 'seed1 file') + .argument('', 'seed2 file') + .action(main); +} async function main(seed1FilePath: string, seed2FilePath: string) { const seed1Binary: Buffer = await fs.readFile(seed1FilePath); diff --git a/src/seed_tools/commands/create_seed.test.ts b/src/seed_tools/commands/create.test.ts similarity index 63% rename from src/seed_tools/commands/create_seed.test.ts rename to src/seed_tools/commands/create.test.ts index 170e8bb7..29fdca85 100644 --- a/src/seed_tools/commands/create_seed.test.ts +++ b/src/seed_tools/commands/create.test.ts @@ -9,19 +9,27 @@ import * as os from 'os'; import * as path from 'path'; import { VariationsSeed } from 'src/proto/generated/variations_seed'; import { wsPath } from '../../base/path_utils'; -import create_seed from './create_seed'; +import create from './create'; -describe('create_seed command', () => { +describe('create command', () => { const testDataDir = wsPath('//src/test/data'); let tempDir: string; + let exitMock: jest.SpyInstance; + let errorMock: jest.SpyInstance; beforeEach(async () => { tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'create_seed-')); + exitMock = jest.spyOn(process, 'exit').mockImplementation((code) => { + throw new Error(`process.exit(${code})`); + }); + errorMock = jest.spyOn(console, 'error').mockImplementation(); }); afterEach(async () => { await fs.rm(tempDir, { recursive: true, force: true }); + exitMock.mockRestore(); + errorMock.mockRestore(); }); describe('valid seeds', () => { @@ -34,14 +42,14 @@ describe('create_seed command', () => { const outputFile = path.join(tempDir, 'output.bin'); const serialNumberPath = path.join(tempDir, 'serial_number.txt'); - await create_seed.parseAsync([ + await create().parseAsync([ 'node', - 'create_seed', + 'create', studiesDir, outputFile, '--mock_serial_number', '1', - '--serial_number_path', + '--output_serial_number_file', serialNumberPath, ]); @@ -64,16 +72,16 @@ describe('create_seed command', () => { const outputFile = path.join(tempDir, 'output.bin'); const serialNumberPath = path.join(tempDir, 'serial_number.txt'); - await create_seed.parseAsync([ + await create().parseAsync([ 'node', - 'create_seed', + 'create', studiesDir, outputFile, '--version', 'test version value', '--mock_serial_number', '1', - '--serial_number_path', + '--output_serial_number_file', serialNumberPath, ]); @@ -101,17 +109,17 @@ describe('create_seed command', () => { const serialNumberPath = path.join(tempDir, 'serial_number.txt'); await expect( - create_seed.parseAsync([ + create().parseAsync([ 'node', - 'create_seed', + 'create', studiesDir, outputFile, '--mock_serial_number', '1', - '--serial_number_path', + '--output_serial_number_file', serialNumberPath, ]), - ).rejects.toThrow(); + ).rejects.toThrowError('process.exit(1)'); }, ); }); @@ -126,25 +134,74 @@ describe('create_seed command', () => { const outputFile = path.join(tempDir, 'output.bin'); const serialNumberPath = path.join(tempDir, 'serial_number.txt'); + await expect( + create().parseAsync([ + 'node', + 'create', + studiesDir, + outputFile, + '--mock_serial_number', + '1', + '--output_serial_number_file', + serialNumberPath, + ]), + ).rejects.toThrowError('process.exit(1)'); + const expectedError = ( await fs.readFile( path.join(testCaseDir, 'expected_error.txt'), 'utf-8', ) ).trim(); + expect(errorMock).toHaveBeenCalledWith( + expect.stringContaining(expectedError), + ); + }, + ); + }); + + describe('unformatted studies', () => { + const unformattedStudiesDir = path.join(testDataDir, 'unformatted_studies'); + + it.each(fs_sync.readdirSync(unformattedStudiesDir))( + 'correctly lints %s', + async (testCase) => { + const testCaseDir = path.join(unformattedStudiesDir, testCase); + const studiesDir = path.join(testCaseDir, 'studies'); + const outputFile = path.join(tempDir, 'output.bin'); + + await expect( + create().parseAsync(['node', 'create', studiesDir, outputFile]), + ).rejects.toThrowError('process.exit(1)'); + + const expectedOutput = await fs.readFile( + path.join(testCaseDir, 'expected_output.txt'), + 'utf-8', + ); + expect(errorMock).toHaveBeenCalledWith( + expect.stringContaining(expectedOutput), + ); + }, + ); + }); + + describe('studies should be valid in invalid_seed test dir', () => { + const invalidSeedsDir = path.join(testDataDir, 'invalid_seeds'); + + it.each(fs_sync.readdirSync(invalidSeedsDir))( + 'correctly lints %s', + async (testCase) => { + const testCaseDir = path.join(invalidSeedsDir, testCase); + const outputFile = path.join(tempDir, 'output.bin'); await expect( - create_seed.parseAsync([ + create().parseAsync([ 'node', - 'create_seed', - studiesDir, + 'create', + path.join(testCaseDir, 'studies'), outputFile, - '--mock_serial_number', - '1', - '--serial_number_path', - serialNumberPath, ]), - ).rejects.toThrow(new RegExp(expectedError)); + ).rejects.toThrowError('process.exit(1)'); }, ); }); diff --git a/src/seed_tools/commands/create.ts b/src/seed_tools/commands/create.ts new file mode 100644 index 00000000..58e32f7b --- /dev/null +++ b/src/seed_tools/commands/create.ts @@ -0,0 +1,66 @@ +// Copyright (c) 2024 The Brave Authors. All rights reserved. +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this file, +// You can obtain one at https://mozilla.org/MPL/2.0/. + +import { Command } from '@commander-js/extra-typings'; +import { createHash } from 'crypto'; +import { promises as fs } from 'fs'; +import { VariationsSeed } from '../../proto/generated/variations_seed'; +import { readStudiesToSeed } from '../utils/studies_to_seed'; + +export default function createCommand() { + return new Command('create') + .description('Create seed.bin from study files') + .argument('', 'path to the directory containing study files') + .argument('', 'output seed file') + .option('--mock_serial_number ', 'mock serial number') + .option( + '--output_serial_number_file ', + 'file path to write the seed serial number', + './serialnumber', + ) + .option('--version ', 'seed version to set') + .action(createSeed); +} + +interface Options { + mock_serial_number?: string; + output_serial_number_file: string; + version?: string; +} + +async function createSeed( + studiesDir: string, + outputSeedFile: string, + options: Options, +) { + const { variationsSeed, errors } = await readStudiesToSeed(studiesDir, false); + + if (errors.length > 0) { + console.error(`Seed validation errors:\n${errors.join('\n---\n')}`); + process.exit(1); + } + + const serialNumber = + options.mock_serial_number ?? generateSerialNumber(variationsSeed); + variationsSeed.serial_number = serialNumber; + + variationsSeed.version = options.version ?? '1'; + + console.log('Seed study count:', variationsSeed.study.length); + const seedBinary = VariationsSeed.toBinary(variationsSeed); + await fs.writeFile(outputSeedFile, seedBinary); + await fs.writeFile(options.output_serial_number_file, serialNumber); + console.log(outputSeedFile, 'created with serial number', serialNumber); +} + +function generateSerialNumber(variationsSeed: VariationsSeed): string { + const seedBinary = VariationsSeed.toBinary(variationsSeed); + const timestamp = String(Date.now()); + const hash = createHash('md5') + .update(seedBinary) + .update(timestamp) + .digest('hex'); + return hash; +} diff --git a/src/seed_tools/commands/create_seed.ts b/src/seed_tools/commands/create_seed.ts deleted file mode 100644 index 66a3d1f8..00000000 --- a/src/seed_tools/commands/create_seed.ts +++ /dev/null @@ -1,94 +0,0 @@ -// Copyright (c) 2024 The Brave Authors. All rights reserved. -// This Source Code Form is subject to the terms of the Mozilla Public -// License, v. 2.0. If a copy of the MPL was not distributed with this file, -// You can obtain one at https://mozilla.org/MPL/2.0/. - -import { Command } from '@commander-js/extra-typings'; -import { createHash } from 'crypto'; -import { promises as fs } from 'fs'; -import * as path from 'path'; -import { - Study_ActivationType, - Study_Consistency, - type Study, -} from '../../proto/generated/study'; -import { VariationsSeed } from '../../proto/generated/variations_seed'; -import * as seed_validation from '../utils/seed_validation'; -import * as study_json_utils from '../utils/study_json_utils'; -import * as study_validation from '../utils/study_validation'; - -export default new Command('create_seed') - .description('Create seed.bin from study files') - .argument('', 'path to a directory containing study files') - .argument('', 'output seed file') - .option('--version ', 'version to set into the seed') - .option( - '--serial_number_path ', - 'file path to write the serial number to', - './serialnumber', - ) - .option('--mock_serial_number ', 'mock serial number') - .action(main); - -interface Options { - mock_serial_number?: string; - serial_number_path?: string; - version?: string; -} - -async function main(studiesDir: string, outputFile: string, options: Options) { - const files: string[] = []; - for (const dirItem of await fs.readdir(studiesDir, { withFileTypes: true })) { - if (!dirItem.isFile()) { - continue; - } - files.push(dirItem.name); - } - files.sort(); - - const variationsSeed: VariationsSeed = { - study: [], - layers: [], - version: options.version ?? '1', - }; - - for (const file of files) { - const filePath = path.join(studiesDir, file); - const studies = await study_json_utils.readStudyFile(filePath); - for (const study of studies) { - study_validation.validateStudy(study, filePath); - setStudyFixedParameters(study); - variationsSeed.study.push(study); - } - } - - seed_validation.validateSeed(variationsSeed); - - const serialNumber = - options.mock_serial_number ?? generateSerialNumber(variationsSeed); - variationsSeed.serial_number = serialNumber; - - const seedBinary = VariationsSeed.toBinary(variationsSeed); - await fs.writeFile(outputFile, seedBinary); - if (options.serial_number_path !== undefined) { - await fs.writeFile(options.serial_number_path, serialNumber); - } - console.log(outputFile, 'created with serial number', serialNumber); - console.log('Study files count:', files.length); - console.log('Seed studies count:', variationsSeed.study.length); -} - -function generateSerialNumber(variationsSeed: VariationsSeed): string { - const seedBinary = VariationsSeed.toBinary(variationsSeed); - const timestamp = String(Date.now()); - const hash = createHash('md5') - .update(seedBinary) - .update(timestamp) - .digest('hex'); - return hash; -} - -function setStudyFixedParameters(study: Study) { - study.activation_type = Study_ActivationType.ACTIVATE_ON_STARTUP; - study.consistency = Study_Consistency.PERMANENT; -} diff --git a/src/seed_tools/commands/lint.test.ts b/src/seed_tools/commands/lint.test.ts new file mode 100644 index 00000000..e56446d3 --- /dev/null +++ b/src/seed_tools/commands/lint.test.ts @@ -0,0 +1,114 @@ +// Copyright (c) 2024 The Brave Authors. All rights reserved. +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this file, +// You can obtain one at https://mozilla.org/MPL/2.0/. + +import * as fs_sync from 'fs'; +import { promises as fs } from 'fs'; +import * as os from 'os'; +import * as path from 'path'; +import { wsPath } from '../../base/path_utils'; +import create from './create'; +import lint from './lint'; + +describe('lint command', () => { + const testDataDir = wsPath('//src/test/data'); + + let tempDir: string; + let exitMock: jest.SpyInstance; + let errorMock: jest.SpyInstance; + + beforeEach(async () => { + tempDir = await fs.mkdtemp(path.join(os.tmpdir(), 'create_seed-')); + exitMock = jest.spyOn(process, 'exit').mockImplementation((code) => { + throw new Error(`process.exit(${code})`); + }); + errorMock = jest.spyOn(console, 'error').mockImplementation(); + }); + + afterEach(async () => { + await fs.rm(tempDir, { recursive: true, force: true }); + exitMock.mockRestore(); + errorMock.mockRestore(); + }); + + describe('lint valid studies', () => { + const unformattedStudiesDir = path.join(testDataDir, 'valid_seeds'); + + it.each(fs_sync.readdirSync(unformattedStudiesDir))( + 'correctly lints %s', + async (testCase) => { + const testCaseDir = path.join(unformattedStudiesDir, testCase); + const studiesDir = path.join(testCaseDir, 'studies'); + + await lint().parseAsync(['node', 'lint', studiesDir]); + + expect(errorMock).toHaveBeenCalledTimes(0); + }, + ); + }); + + describe('lint unformatted studies', () => { + const unformattedStudiesDir = path.join(testDataDir, 'unformatted_studies'); + + it.each(fs_sync.readdirSync(unformattedStudiesDir))( + 'correctly lints %s', + async (testCase) => { + const testCaseDir = path.join(unformattedStudiesDir, testCase); + const studiesDir = path.join(testCaseDir, 'studies'); + + await expect( + lint().parseAsync(['node', 'lint', studiesDir]), + ).rejects.toThrowError('process.exit(1)'); + + const expectedOutput = await fs.readFile( + path.join(testCaseDir, 'expected_output.txt'), + 'utf-8', + ); + expect(errorMock).toHaveBeenCalledWith( + expect.stringContaining(expectedOutput), + ); + }, + ); + }); + + describe('fix unformatted studies', () => { + const unformattedStudiesDir = path.join(testDataDir, 'unformatted_studies'); + + it.each(fs_sync.readdirSync(unformattedStudiesDir))( + 'correctly fixes %s', + async (testCase) => { + const testCaseDir = path.join(unformattedStudiesDir, testCase); + const studiesDir = path.join(testCaseDir, 'studies'); + const tempStudiesDir = path.join(tempDir, 'studies'); + const outputFile = path.join(tempDir, 'output.bin'); + + // copy the unformatted studies to a temp dir + await fs.mkdir(tempStudiesDir); + await fs.copyFile( + path.join(studiesDir, 'TestStudy.json'), + path.join(tempStudiesDir, 'TestStudy.json'), + ); + + // lint should fail. + await expect( + lint().parseAsync(['node', 'lint', tempStudiesDir]), + ).rejects.toThrowError('process.exit(1)'); + + // Fix studies. + await lint().parseAsync(['node', 'lint', tempStudiesDir, '--fix']); + + // lint should not fail. + await lint().parseAsync(['node', 'lint', tempStudiesDir]); + + // Seed creation should not fail. + await create().parseAsync([ + 'node', + 'create', + tempStudiesDir, + outputFile, + ]); + }, + ); + }); +}); diff --git a/src/seed_tools/commands/lint.ts b/src/seed_tools/commands/lint.ts new file mode 100644 index 00000000..9108deae --- /dev/null +++ b/src/seed_tools/commands/lint.ts @@ -0,0 +1,33 @@ +// Copyright (c) 2024 The Brave Authors. All rights reserved. +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this file, +// You can obtain one at https://mozilla.org/MPL/2.0/. + +import { Command } from '@commander-js/extra-typings'; +import { readStudiesToSeed } from '../utils/studies_to_seed'; + +export default function createCommand() { + return new Command('lint') + .description('Lint study files without creating seed.bin') + .argument('', 'path to the directory containing study files') + .option('--fix', 'fix format errors in-place') + .action(lintStudies); +} + +interface Options { + fix?: true; +} + +async function lintStudies(studiesDir: string, options: Options) { + const { variationsSeed, errors } = await readStudiesToSeed( + studiesDir, + options.fix, + ); + + if (errors.length > 0) { + console.error(`Lint errors:\n${errors.join('\n---\n')}`); + process.exit(1); + } + + console.log('Lint successful. Study count:', variationsSeed.study.length); +} diff --git a/src/seed_tools/commands/split_seed_json.ts b/src/seed_tools/commands/split_seed_json.ts index f459ce5c..d93b293f 100644 --- a/src/seed_tools/commands/split_seed_json.ts +++ b/src/seed_tools/commands/split_seed_json.ts @@ -10,11 +10,13 @@ import { type Study } from '../../proto/generated/study'; import { VariationsSeed } from '../../proto/generated/variations_seed'; import * as study_json_utils from '../utils/study_json_utils'; -export default new Command('split_seed_json') - .description('Split seed.json into study files') - .argument('', 'path to seed.json') - .argument('', 'output directory') - .action(main); +export default function createCommand() { + return new Command('split_seed_json') + .description('Split seed.json into study files') + .argument('', 'path to seed.json') + .argument('', 'output directory') + .action(main); +} async function main(seedPath: string, outputDir: string) { const seedJson = preprocessSeedJson( diff --git a/src/seed_tools/commands/validate_seed.test.ts b/src/seed_tools/commands/validate_seed.test.ts deleted file mode 100644 index 3a9ffc11..00000000 --- a/src/seed_tools/commands/validate_seed.test.ts +++ /dev/null @@ -1,55 +0,0 @@ -// Copyright (c) 2024 The Brave Authors. All rights reserved. -// This Source Code Form is subject to the terms of the Mozilla Public -// License, v. 2.0. If a copy of the MPL was not distributed with this file, -// You can obtain one at https://mozilla.org/MPL/2.0/. - -import * as fs_sync from 'fs'; -import * as path from 'path'; -import { wsPath } from '../../base/path_utils'; -import validate_seed from './validate_seed'; - -describe('validate_seed command', () => { - const testDataDir = wsPath('//src/test/data'); - - let errorMock: jest.SpyInstance; - let exitMock: jest.SpyInstance; - - beforeEach(() => { - errorMock = jest.spyOn(console, 'error').mockImplementation(); - exitMock = jest.spyOn(process, 'exit').mockImplementation(); - }); - - afterEach(() => { - jest.restoreAllMocks(); - }); - - describe('valid seeds', () => { - const validSeedsDir = path.join(testDataDir, 'valid_seeds'); - it.each(fs_sync.readdirSync(validSeedsDir))( - 'correctly validates %s', - async (testCase) => { - const testCaseDir = path.join(validSeedsDir, testCase); - const seedBin = path.join(testCaseDir, 'expected_seed.bin'); - - await validate_seed.parseAsync(['node', 'validate_seed', seedBin]); - - expect(errorMock).toHaveBeenCalledTimes(0); - expect(exitMock).toHaveBeenCalledWith(0); - }, - ); - }); - - test('invalid seed', async () => { - const seedBin = path.join(testDataDir, 'invalid_seed.bin'); - expect(fs_sync.existsSync(seedBin)).toBe(true); - - await validate_seed.parseAsync(['node', 'validate_seed', seedBin]); - - expect(errorMock).toHaveBeenCalledWith( - expect.stringContaining( - 'Total probability is not 100 for study AllowCertainClientHintsStudy', - ), - ); - expect(exitMock).toHaveBeenCalledWith(1); - }); -}); diff --git a/src/seed_tools/commands/validate_seed.ts b/src/seed_tools/commands/validate_seed.ts deleted file mode 100644 index f8d28de8..00000000 --- a/src/seed_tools/commands/validate_seed.ts +++ /dev/null @@ -1,44 +0,0 @@ -// Copyright (c) 2024 The Brave Authors. All rights reserved. -// This Source Code Form is subject to the terms of the Mozilla Public -// License, v. 2.0. If a copy of the MPL was not distributed with this file, -// You can obtain one at https://mozilla.org/MPL/2.0/. - -import { Command } from '@commander-js/extra-typings'; -import { promises as fs } from 'fs'; -import { VariationsSeed } from '../../proto/generated/variations_seed'; -import * as seed_validation from '../utils/seed_validation'; -import * as study_validation from '../utils/study_validation'; - -export default new Command('validate_seed') - .description('Validates seed.bin') - .argument('', 'path to a seed protobuf') - .action(main); - -async function main(seedFilePath: string) { - const variationsSeed = VariationsSeed.fromBinary( - await fs.readFile(seedFilePath, { encoding: null }), - ); - const errors = []; - for (const study of variationsSeed.study) { - const filePath = `${study.name}.json`; - for (const error of study_validation.validateStudyReturnErrors( - study, - filePath, - )) { - errors.push(error); - } - } - - for (const error of seed_validation.validateSeedReturnErrors( - variationsSeed, - )) { - errors.push(error); - } - - console.log('Seed studies count:', variationsSeed.study.length); - if (errors.length > 0) { - console.error(`Seed validation errors:\n${errors.join('\n---\n')}`); - } - - process.exit(errors.length > 0 ? 1 : 0); -} diff --git a/src/seed_tools/seed_tools.ts b/src/seed_tools/seed_tools.ts index 3a43331e..16541469 100644 --- a/src/seed_tools/seed_tools.ts +++ b/src/seed_tools/seed_tools.ts @@ -5,18 +5,16 @@ import { program } from '@commander-js/extra-typings'; -import check_study from './commands/check_study'; import compare_seeds from './commands/compare_seeds'; -import create_seed from './commands/create_seed'; +import create from './commands/create'; +import lint from './commands/lint'; import split_seed_json from './commands/split_seed_json'; -import validate_seed from './commands/validate_seed'; program .name('seed_tools') .description('Seed tools for manipulating study files.') - .addCommand(check_study) - .addCommand(compare_seeds) - .addCommand(create_seed) - .addCommand(split_seed_json) - .addCommand(validate_seed) + .addCommand(compare_seeds()) + .addCommand(create()) + .addCommand(lint()) + .addCommand(split_seed_json()) .parse(); diff --git a/src/seed_tools/utils/file_utils.ts b/src/seed_tools/utils/file_utils.ts new file mode 100644 index 00000000..be7c2ff2 --- /dev/null +++ b/src/seed_tools/utils/file_utils.ts @@ -0,0 +1,10 @@ +// Copyright (c) 2024 The Brave Authors. All rights reserved. +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this file, +// You can obtain one at https://mozilla.org/MPL/2.0/. + +import path from 'path'; + +export function getFileBaseName(filePath: string): string { + return path.basename(filePath, path.extname(filePath)); +} diff --git a/src/seed_tools/utils/seed_validation.test.ts b/src/seed_tools/utils/seed_validation.test.ts index 86b90599..97617556 100644 --- a/src/seed_tools/utils/seed_validation.test.ts +++ b/src/seed_tools/utils/seed_validation.test.ts @@ -5,9 +5,9 @@ import { Study } from '../../proto/generated/study'; import { type VariationsSeed } from '../../proto/generated/variations_seed'; -import { validateSeed } from './seed_validation'; +import { getSeedErrors } from './seed_validation'; -describe('validateSeed', () => { +describe('getSeedErrors', () => { describe('checkOverlappingStudies', () => { function toFilterDate(date: string): number { return new Date(date).getTime() / 1000; @@ -18,9 +18,11 @@ describe('validateSeed', () => { name, experiment: [ { + name: 'experiment1', feature_association: { enable_feature: ['feature1'], }, + probability_weight: 100, }, ], }; @@ -406,14 +408,13 @@ describe('validateSeed', () => { ], } as unknown as VariationsSeed; + const errors: string[] = getSeedErrors(seed); if (testCase.expectedOverlapped) { - expect(() => { - validateSeed(seed); - }).toThrowError('overlaps in studies'); + expect(errors).toContainEqual( + expect.stringContaining('overlaps in studies'), + ); } else { - expect(() => { - validateSeed(seed); - }).not.toThrow(); + expect(errors).toEqual([]); } }); }); diff --git a/src/seed_tools/utils/seed_validation.ts b/src/seed_tools/utils/seed_validation.ts index 76ee61f1..9dd887cd 100644 --- a/src/seed_tools/utils/seed_validation.ts +++ b/src/seed_tools/utils/seed_validation.ts @@ -5,22 +5,27 @@ import assert from 'assert'; import DefaultMap from '../../base/containers/default_map'; -import { type Study_Filter } from '../../proto/generated/study'; +import { Study, type Study_Filter } from '../../proto/generated/study'; import { type VariationsSeed } from '../../proto/generated/variations_seed'; import ProcessedStudy from './processed_study'; import * as study_json_utils from './study_json_utils'; - -// Validate a seed for common errors. Throws an error if any is found. -export function validateSeed(seed: VariationsSeed) { - const errors = validateSeedReturnErrors(seed); - if (errors.length > 0) { - throw new Error(`Error validating seed:\n${errors.join('\n')}`); - } -} +import * as study_validation from './study_validation'; // Validate a seed for common errors. Returns an array of error messages. -export function validateSeedReturnErrors(seed: VariationsSeed): string[] { +export function getSeedErrors( + seed: VariationsSeed, + studyFileBaseNameMap?: Map, +): string[] { const errors: string[] = []; + for (const study of seed.study) { + errors.push( + ...study_validation.getStudyErrors( + study, + studyFileBaseNameMap?.get(study) ?? study.name, + ), + ); + } + const validators = [checkOverlappingStudies]; for (const validator of validators) { try { @@ -113,7 +118,7 @@ function checkOverlappingStudies(seed: VariationsSeed): string[] { if (!hasFilterDifference) { errors.push( `Feature ${featureName} overlaps in studies. Check your filters:\n` + - `${study_json_utils.stringifyStudyArray([study1.study, study2.study])}`, + `${study_json_utils.stringifyStudies([study1.study, study2.study])}`, ); } } diff --git a/src/seed_tools/utils/studies_to_seed.ts b/src/seed_tools/utils/studies_to_seed.ts new file mode 100644 index 00000000..e9acc81c --- /dev/null +++ b/src/seed_tools/utils/studies_to_seed.ts @@ -0,0 +1,115 @@ +// Copyright (c) 2024 The Brave Authors. All rights reserved. +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this file, +// You can obtain one at https://mozilla.org/MPL/2.0/. + +import { promises as fs } from 'fs'; +import * as path from 'path'; +import { + Study_ActivationType, + Study_Consistency, + type Study, +} from '../../proto/generated/study'; +import { VariationsSeed } from '../../proto/generated/variations_seed'; +import diffStrings from '../utils/diff_strings'; +import * as file_utils from '../utils/file_utils'; +import * as seed_validation from '../utils/seed_validation'; +import * as study_json_utils from '../utils/study_json_utils'; + +export async function readStudiesToSeed( + studiesDir: string, + fix = false, +): Promise<{ + variationsSeed: VariationsSeed; + errors: string[]; +}> { + const { studies, studyFileBaseNameMap, errors } = + await readStudiesFromDirectory(studiesDir, fix); + + const variationsSeed: VariationsSeed = { + study: studies, + layers: [], + }; + + errors.push( + ...seed_validation.getSeedErrors(variationsSeed, studyFileBaseNameMap), + ); + + return { variationsSeed, errors }; +} + +async function readStudiesFromDirectory( + studiesDir: string, + fix: boolean, +): Promise<{ + studies: Study[]; + studyFileBaseNameMap: Map; + errors: string[]; +}> { + const files = (await fs.readdir(studiesDir)).sort(); + + const studies: Study[] = []; + const studyFileBaseNameMap = new Map(); + const errors: string[] = []; + + for (const file of files) { + const filePath = path.join(studiesDir, file); + const readStudyFileResult = await study_json_utils.readStudyFile(filePath); + errors.push(...readStudyFileResult.errors); + if (readStudyFileResult.errors.length === 0) { + errors.push( + ...(await checkAndOptionallyFixFormat( + filePath, + readStudyFileResult.studies, + readStudyFileResult.studyFileContent, + fix, + )), + ); + } + if (readStudyFileResult.errors.length > 0) { + continue; + } + for (const study of readStudyFileResult.studies) { + setStudyDefaultParameters(study); + studies.push(study); + studyFileBaseNameMap.set(study, file_utils.getFileBaseName(filePath)); + } + } + + return { studies, studyFileBaseNameMap, errors }; +} + +async function checkAndOptionallyFixFormat( + studyFilePath: string, + studies: Study[], + studyArrayString: string, + fix: boolean, +): Promise { + const errors: string[] = []; + const stringifiedStudies = study_json_utils.stringifyStudies(studies); + if (stringifiedStudies !== studyArrayString) { + if (fix) { + await fs.writeFile(studyFilePath, stringifiedStudies); + } else { + errors.push( + `Format required:\n` + + (await diffStrings( + studyArrayString, + stringifiedStudies, + studyFilePath, + studyFilePath + '.formatted', + )), + ); + } + } + return errors; +} + +function setStudyDefaultParameters(study: Study) { + if (study.activation_type === undefined) { + study.activation_type = Study_ActivationType.ACTIVATE_ON_STARTUP; + } + if (study.consistency === undefined) { + study.consistency = Study_Consistency.PERMANENT; + } +} diff --git a/src/seed_tools/utils/study_json_utils.test.ts b/src/seed_tools/utils/study_json_utils.test.ts index 850b2ac6..610d6aad 100644 --- a/src/seed_tools/utils/study_json_utils.test.ts +++ b/src/seed_tools/utils/study_json_utils.test.ts @@ -8,9 +8,9 @@ import { Study_Channel, Study_Platform, } from '../../proto/generated/study'; -import { parseStudyArray, stringifyStudyArray } from './study_json_utils'; +import { parseStudies, stringifyStudies } from './study_json_utils'; -describe('stringifyStudyArray', () => { +describe('stringifyStudies', () => { it('should convert start_date and end_date to ISO string', () => { const startDate = new Date('2022-01-01T00:00:00Z'); const endDate = new Date('2022-12-31T23:59:59Z'); @@ -25,7 +25,7 @@ describe('stringifyStudyArray', () => { { ignoreUnknownFields: false }, ); - const stringifiedStudyArray = stringifyStudyArray([study]); + const stringifiedStudyArray = stringifyStudies([study]); expect(JSON.parse(stringifiedStudyArray)).toEqual([ { name: 'study', @@ -48,7 +48,7 @@ describe('stringifyStudyArray', () => { { ignoreUnknownFields: false }, ); - const stringifiedStudyArray = stringifyStudyArray([study]); + const stringifiedStudyArray = stringifyStudies([study]); expect(JSON.parse(stringifiedStudyArray)).toEqual([ { name: 'study', @@ -80,7 +80,7 @@ describe('stringifyStudyArray', () => { { ignoreUnknownFields: false }, ); - const stringifiedStudyArray = stringifyStudyArray([study]); + const stringifiedStudyArray = stringifyStudies([study]); expect(JSON.parse(stringifiedStudyArray)).toEqual([ { name: 'BraveHorizontalTabsUpdateEnabledStudy', @@ -114,7 +114,7 @@ describe('stringifyStudyArray', () => { { ignoreUnknownFields: false }, ); - const stringifiedStudyArray = stringifyStudyArray([study], { + const stringifiedStudyArray = stringifyStudies([study], { isChromium: true, }); expect(JSON.parse(stringifiedStudyArray)).toEqual([ @@ -129,7 +129,7 @@ describe('stringifyStudyArray', () => { }); }); -describe('parseStudyArray', () => { +describe('parseStudies', () => { it('should convert ISO string start_date and end_date to Unix timestamp', () => { const startDate = '2022-01-01T00:00:00.000Z'; const endDate = '2022-12-31T23:59:59.999Z'; @@ -144,7 +144,7 @@ describe('parseStudyArray', () => { }, ]); - const parsedStudyArray = parseStudyArray(study); + const parsedStudyArray = parseStudies(study); expect(parsedStudyArray[0].filter?.start_date).toEqual( BigInt(Math.floor(new Date(startDate).getTime() / 1000)), ); @@ -155,7 +155,7 @@ describe('parseStudyArray', () => { it('should throw an error for invalid start_date or end_date format', () => { const parseStudyWithFilter = (filter: any) => { - return parseStudyArray( + return parseStudies( JSON.stringify([ { name: 'study', @@ -186,7 +186,7 @@ describe('parseStudyArray', () => { }, ]); - const parsedStudyArray = parseStudyArray(study); + const parsedStudyArray = parseStudies(study); expect(parsedStudyArray[0].filter?.channel).toEqual([ Study_Channel.CANARY, Study_Channel.BETA, @@ -205,7 +205,7 @@ describe('parseStudyArray', () => { }, ]); - const parsedStudyArray = parseStudyArray(study); + const parsedStudyArray = parseStudies(study); expect(parsedStudyArray[0].filter?.channel).toEqual([ Study_Channel.STABLE, Study_Channel.BETA, @@ -224,14 +224,14 @@ describe('parseStudyArray', () => { Study.fromJson({ name: 'Study 2' }), ]; - const result = parseStudyArray(studyArrayString); + const result = parseStudies(studyArrayString); expect(result).toEqual(expectedStudyArray); }); it('should throw an error if the study is not array', () => { const invalidStudyArrayString = '{}'; - expect(() => parseStudyArray(invalidStudyArrayString)).toThrowError( + expect(() => parseStudies(invalidStudyArrayString)).toThrowError( 'Root element must be an array', ); }); @@ -239,7 +239,7 @@ describe('parseStudyArray', () => { it('should throw an error if the study array string is invalid', () => { const invalidStudyArrayString = 'invalid'; - expect(() => parseStudyArray(invalidStudyArrayString)).toThrowError( + expect(() => parseStudies(invalidStudyArrayString)).toThrowError( 'Unexpected token', ); }); @@ -247,7 +247,7 @@ describe('parseStudyArray', () => { it('should throw on unknown fields when parsing studies', () => { const studyArrayString = '[{"name":"Study 1","unknownField":"value"}]'; - expect(() => parseStudyArray(studyArrayString)).toThrowError( + expect(() => parseStudies(studyArrayString)).toThrowError( 'Found unknown field while reading variations.Study from JSON format. JSON key: unknownField', ); }); @@ -255,7 +255,7 @@ describe('parseStudyArray', () => { it('should throw on invalid field types when parsing studies', () => { const studyArrayString = '[{"name":"Study 1","experiment":"value"}]'; - expect(() => parseStudyArray(studyArrayString)).toThrowError( + expect(() => parseStudies(studyArrayString)).toThrowError( 'Cannot parse JSON string for variations.Study#experiment', ); }); @@ -264,7 +264,7 @@ describe('parseStudyArray', () => { const studyArrayString = '[{"name":"Study 1","experiment":[{"probability_weight":"abc"}]}]'; - expect(() => parseStudyArray(studyArrayString)).toThrowError( + expect(() => parseStudies(studyArrayString)).toThrowError( 'Cannot parse JSON string for variations.Study.Experiment#probability_weight - invalid uint 32: NaN', ); }); @@ -279,7 +279,7 @@ describe('parseStudyArray', () => { }, ]); - const parsedStudyArray = parseStudyArray(study, { isChromium: true }); + const parsedStudyArray = parseStudies(study, { isChromium: true }); expect(parsedStudyArray[0].filter?.channel).toEqual([ Study_Channel.CANARY, Study_Channel.BETA, diff --git a/src/seed_tools/utils/study_json_utils.ts b/src/seed_tools/utils/study_json_utils.ts index 46e5ce8c..914b434c 100644 --- a/src/seed_tools/utils/study_json_utils.ts +++ b/src/seed_tools/utils/study_json_utils.ts @@ -13,27 +13,21 @@ export interface Options { export async function readStudyFile( studyFilePath: string, options?: Options, -): Promise { - const result = await readStudyFileReturnWithError(studyFilePath, options); - if (result instanceof Error) { - throw result; - } else { - return result[0]; - } -} - -export async function readStudyFileReturnWithError( - studyFilePath: string, - options?: Options, -): Promise<[Study[], string] | Error> { +): Promise<{ + studies: Study[]; + studyFileContent: string; + errors: string[]; +}> { + let studies: Study[] = []; + let studyFileContent = ''; try { - const studyArrayString = await fs.readFile(studyFilePath, 'utf8'); - const studyArray = parseStudyArray(studyArrayString, options); - return [studyArray, studyArrayString]; + studyFileContent = await fs.readFile(studyFilePath, 'utf8'); + studies = parseStudies(studyFileContent, options); + return { studies, studyFileContent, errors: [] }; } catch (e) { if (e instanceof Error) { e.message += ` (${studyFilePath})`; - return e; + return { studies, studyFileContent, errors: [e.message] }; } // Rethrow non-Error exceptions. throw e; @@ -41,14 +35,14 @@ export async function readStudyFileReturnWithError( } export async function writeStudyFile( - studyArray: Study[], + studies: Study[], studyFilePath: string, options?: Options, ) { - await fs.writeFile(studyFilePath, stringifyStudyArray(studyArray, options)); + await fs.writeFile(studyFilePath, stringifyStudies(studies, options)); } -export function parseStudyArray( +export function parseStudies( studyArrayString: string, options?: Options, ): Study[] { @@ -71,12 +65,9 @@ export function parseStudyArray( return studyArray; } -export function stringifyStudyArray( - studyArray: Study[], - options?: Options, -): string { +export function stringifyStudies(studies: Study[], options?: Options): string { const jsonStudies: any[] = []; - for (const study of studyArray) { + for (const study of studies) { const jsonStudy = Study.toJson(study, { emitDefaultValues: false, enumAsInteger: false, diff --git a/src/seed_tools/utils/study_validation.test.ts b/src/seed_tools/utils/study_validation.test.ts index c0c42205..0190f0d4 100644 --- a/src/seed_tools/utils/study_validation.test.ts +++ b/src/seed_tools/utils/study_validation.test.ts @@ -7,10 +7,10 @@ import { describe, expect, test } from '@jest/globals'; import { Study } from '../../proto/generated/study'; import * as study_validation from './study_validation'; -describe('validateStudy', () => { - const studyFilePath = '/path/to/study.json'; +describe('getStudyErrors', () => { + const studyFileBaseName = 'study'; - test('should not throw an error if study is valid', () => { + test('should not error if study is valid', () => { const study = Study.fromJson({ name: 'study', experiment: [ @@ -40,37 +40,41 @@ describe('validateStudy', () => { }, }); - expect(() => { - study_validation.validateStudy(study, studyFilePath); - }).not.toThrow(); + expect(study_validation.getStudyErrors(study, studyFileBaseName)).toEqual( + [], + ); }); - test('should throw an error if study name does not match file name', () => { + test('should error if study name does not match file name', () => { const study = Study.fromJson({ name: 'study1', experiment: [], }); - expect(() => { - study_validation.validateStudy(study, studyFilePath); - }).toThrowError('Study name study1 does not match file name'); + expect( + study_validation.getStudyErrors(study, studyFileBaseName), + ).toContainEqual( + expect.stringContaining('Study name study1 does not match file name'), + ); }); test.each(['study_😀', 'study_,', 'study_<', 'study_*'])( - 'should throw an error if study name has invalid char %s', + 'should error if study name has invalid char %s', (studyName) => { const study = Study.fromJson({ name: studyName, experiment: [], }); - expect(() => { - study_validation.validateStudy(study, studyFilePath); - }).toThrowError(`Invalid study name: ${studyName}`); + expect( + study_validation.getStudyErrors(study, studyFileBaseName), + ).toContainEqual( + expect.stringContaining(`Invalid study name: ${studyName}`), + ); }, ); - test('should throw an error if layer is set', () => { + test('should error if layer is set', () => { const study = Study.fromJson({ name: 'study', layer: { @@ -79,12 +83,14 @@ describe('validateStudy', () => { }, }); - expect(() => { - study_validation.validateStudy(study, studyFilePath); - }).toThrowError('Layers are currently not supported'); + expect( + study_validation.getStudyErrors(study, studyFileBaseName), + ).toContainEqual( + expect.stringContaining('Layers are currently not supported'), + ); }); - test('should throw an error if experiment name is not defined', () => { + test('should error if experiment name is not defined', () => { const study = Study.fromJson({ name: 'study', experiment: [ @@ -95,13 +101,17 @@ describe('validateStudy', () => { ], }); - expect(() => { - study_validation.validateStudy(study, studyFilePath); - }).toThrowError('Experiment name is not defined for study: study'); + expect( + study_validation.getStudyErrors(study, studyFileBaseName), + ).toContainEqual( + expect.stringContaining( + 'Experiment name is not defined for study: study', + ), + ); }); test.each(['exp😀', 'exp<', 'exp*'])( - 'should throw an error if experiment name has invalid char %s', + 'should error if experiment name has invalid char %s', (experimentName) => { const study = Study.fromJson({ name: 'study', @@ -113,13 +123,15 @@ describe('validateStudy', () => { ], }); - expect(() => { - study_validation.validateStudy(study, studyFilePath); - }).toThrowError(`Invalid experiment name: ${experimentName}`); + expect( + study_validation.getStudyErrors(study, studyFileBaseName), + ).toContainEqual( + expect.stringContaining(`Invalid experiment name: ${experimentName}`), + ); }, ); - test('should not throw if experiment name has comma', () => { + test('should not error if experiment name has comma', () => { const study = Study.fromJson({ name: 'study', experiment: [ @@ -130,10 +142,12 @@ describe('validateStudy', () => { ], }); - study_validation.validateStudy(study, studyFilePath); + expect(study_validation.getStudyErrors(study, studyFileBaseName)).toEqual( + [], + ); }); - test('should throw an error if duplicate experiment names are found', () => { + test('should error if duplicate experiment names are found', () => { const study = Study.fromJson({ name: 'study', experiment: [ @@ -152,12 +166,14 @@ describe('validateStudy', () => { ], }); - expect(() => { - study_validation.validateStudy(study, studyFilePath); - }).toThrowError('Duplicate experiment name: experiment1'); + expect( + study_validation.getStudyErrors(study, studyFileBaseName), + ).toContainEqual( + expect.stringContaining('Duplicate experiment name: experiment1'), + ); }); - test('should throw an error if feature name is not defined', () => { + test('should error if feature name is not defined', () => { const study = Study.fromJson({ name: 'study', experiment: [ @@ -171,13 +187,17 @@ describe('validateStudy', () => { ], }); - expect(() => { - study_validation.validateStudy(study, studyFilePath); - }).toThrowError('Feature name is not defined for experiment: experiment'); + expect( + study_validation.getStudyErrors(study, studyFileBaseName), + ).toContainEqual( + expect.stringContaining( + 'Feature name is not defined for experiment: experiment', + ), + ); }); test.each(['feature😀', 'feature,', 'feature<', 'feature*'])( - 'should throw an error if feature name has invalid char %s', + 'should error if feature name has invalid char %s', (featureName) => { const featureAssociations = [ { enable_feature: [featureName] }, @@ -197,16 +217,38 @@ describe('validateStudy', () => { ], }); - expect(() => { - study_validation.validateStudy(study, studyFilePath); - }).toThrowError( - `Invalid feature name for experiment experiment: ${featureName}`, + expect( + study_validation.getStudyErrors(study, studyFileBaseName), + ).toContainEqual( + expect.stringContaining( + `Invalid feature name for experiment experiment: ${featureName}`, + ), ); } }, ); - test('should not throw if forcing flag is correct', () => { + test('should error if feature is duplicated', () => { + const study = Study.fromJson({ + name: 'study', + experiment: [ + { + name: 'experiment1', + probability_weight: 100, + feature_association: { + enable_feature: ['Feature'], + disable_feature: ['Feature'], + }, + }, + ], + }); + + expect( + study_validation.getStudyErrors(study, studyFileBaseName), + ).toContainEqual(expect.stringContaining(`Duplicate feature name`)); + }); + + test('should not error if forcing flag is correct', () => { const study = Study.fromJson({ name: 'study', experiment: [ @@ -218,11 +260,13 @@ describe('validateStudy', () => { ], }); - study_validation.validateStudy(study, studyFilePath); + expect(study_validation.getStudyErrors(study, studyFileBaseName)).toEqual( + [], + ); }); test.each(['Hello', ''])( - 'should throw an error if forcing flag is incorrect %s', + 'should error if forcing flag is incorrect %s', (forcingFlag) => { const study = Study.fromJson({ name: 'study', @@ -235,9 +279,13 @@ describe('validateStudy', () => { ], }); - expect(() => { - study_validation.validateStudy(study, studyFilePath); - }).toThrowError('Invalid forcing flag for experiment experiment1'); + expect( + study_validation.getStudyErrors(study, studyFileBaseName), + ).toContainEqual( + expect.stringContaining( + 'Invalid forcing flag for experiment experiment1', + ), + ); }, ); @@ -275,10 +323,12 @@ describe('validateStudy', () => { const study = Study.fromJson(studyJson); - expect(() => { - study_validation.validateStudy(study, studyFilePath); - }).toThrowError( - 'Forcing feature_on, feature_off and flag are mutually exclusive', + expect( + study_validation.getStudyErrors(study, studyFileBaseName), + ).toContainEqual( + expect.stringContaining( + 'Forcing feature_on, feature_off and flag are mutually exclusive', + ), ); }, ); @@ -288,7 +338,7 @@ describe('validateStudy', () => { [false, true, false], [false, false, true], ])( - 'should not throw on correct forcing options use', + 'should not error on correct forcing options use', (forcingFeatureOn, forcingFeatureOff, forcingFlag) => { const studyJson = { name: 'study', @@ -316,11 +366,13 @@ describe('validateStudy', () => { const study = Study.fromJson(studyJson); - study_validation.validateStudy(study, studyFilePath); + expect(study_validation.getStudyErrors(study, studyFileBaseName)).toEqual( + [], + ); }, ); - test('should throw an error if google_web_experiment/trigger_id conflict', () => { + test('should error if google_web_experiment/trigger_id conflict', () => { const study = Study.fromJson({ name: 'study', experiment: [ @@ -333,14 +385,16 @@ describe('validateStudy', () => { ], }); - expect(() => { - study_validation.validateStudy(study, studyFilePath); - }).toThrowError( - 'Experiment experiment1 has both google_web_experiment_id and web_trigger_experiment_id', + expect( + study_validation.getStudyErrors(study, studyFileBaseName), + ).toContainEqual( + expect.stringContaining( + 'Experiment experiment1 has both google_web_experiment_id and web_trigger_experiment_id', + ), ); }); - test('should throw an error if param name is empty', () => { + test('should error if param name is empty', () => { const study = Study.fromJson({ name: 'study', experiment: [ @@ -357,12 +411,14 @@ describe('validateStudy', () => { ], }); - expect(() => { - study_validation.validateStudy(study, studyFilePath); - }).toThrowError('Empty param name in experiment experiment1'); + expect( + study_validation.getStudyErrors(study, studyFileBaseName), + ).toContainEqual( + expect.stringContaining('Empty param name in experiment experiment1'), + ); }); - test('should throw an error if params conflict', () => { + test('should error if params conflict', () => { const study = Study.fromJson({ name: 'study', experiment: [ @@ -383,12 +439,16 @@ describe('validateStudy', () => { ], }); - expect(() => { - study_validation.validateStudy(study, studyFilePath); - }).toThrowError('Duplicate param name: test in experiment experiment1'); + expect( + study_validation.getStudyErrors(study, studyFileBaseName), + ).toContainEqual( + expect.stringContaining( + 'Duplicate param name: test in experiment experiment1', + ), + ); }); - test('should throw an error if default_experiment_name not found', () => { + test('should error if default_experiment_name not found', () => { const study = Study.fromJson({ name: 'study', experiment: [ @@ -400,12 +460,16 @@ describe('validateStudy', () => { default_experiment_name: 'DefaultExp', }); - expect(() => { - study_validation.validateStudy(study, studyFilePath); - }).toThrowError('Missing default experiment: DefaultExp in study study'); + expect( + study_validation.getStudyErrors(study, studyFileBaseName), + ).toContainEqual( + expect.stringContaining( + 'Missing default experiment: DefaultExp in study study', + ), + ); }); - test('should throw an error if total probability is not 100', () => { + test('should error if total probability is not 100', () => { const study = Study.fromJson({ name: 'study', experiment: [ @@ -424,12 +488,14 @@ describe('validateStudy', () => { ], }); - expect(() => { - study_validation.validateStudy(study, studyFilePath); - }).toThrowError('Total probability is not 100 for study study'); + expect( + study_validation.getStudyErrors(study, studyFileBaseName), + ).toContainEqual( + expect.stringContaining('Total probability is not 100 for study study'), + ); }); - test('should throw an error if conflicting filter properties are found', () => { + test('should error if conflicting filter properties are found', () => { const study = Study.fromJson({ name: 'study', experiment: [ @@ -444,12 +510,14 @@ describe('validateStudy', () => { }, }); - expect(() => { - study_validation.validateStudy(study, studyFilePath); - }).toThrowError('Filter conflict: exclude_locale and locale'); + expect( + study_validation.getStudyErrors(study, studyFileBaseName), + ).toContainEqual( + expect.stringContaining('Filter conflict: exclude_locale and locale'), + ); }); - test('should not throw if conflicting filter is empty', () => { + test('should not error if conflicting filter is empty', () => { const study = Study.fromJson({ name: 'study', experiment: [ @@ -464,10 +532,12 @@ describe('validateStudy', () => { }, }); - study_validation.validateStudy(study, studyFilePath); + expect(study_validation.getStudyErrors(study, studyFileBaseName)).toEqual( + [], + ); }); - test('should throw an error if version range is invalid', () => { + test('should error if version range is invalid', () => { const study = Study.fromJson({ name: 'study', experiment: [ @@ -482,12 +552,12 @@ describe('validateStudy', () => { }, }); - expect(() => { - study_validation.validateStudy(study, studyFilePath); - }).toThrowError('Invalid version range'); + expect( + study_validation.getStudyErrors(study, studyFileBaseName), + ).toContainEqual(expect.stringContaining('Invalid version range')); }); - test('should throw an error if version is invalid', () => { + test('should error if version is invalid', () => { const study = Study.fromJson({ name: 'study', experiment: [ @@ -501,12 +571,14 @@ describe('validateStudy', () => { }, }); - expect(() => { - study_validation.validateStudy(study, studyFilePath); - }).toThrowError('contains non-numeric characters'); + expect( + study_validation.getStudyErrors(study, studyFileBaseName), + ).toContainEqual( + expect.stringContaining('contains non-numeric characters'), + ); }); - test('should throw an error if os version range is invalid', () => { + test('should error if os version range is invalid', () => { const study = Study.fromJson({ name: 'study', experiment: [ @@ -521,12 +593,12 @@ describe('validateStudy', () => { }, }); - expect(() => { - study_validation.validateStudy(study, studyFilePath); - }).toThrowError('Invalid os_version range'); + expect( + study_validation.getStudyErrors(study, studyFileBaseName), + ).toContainEqual(expect.stringContaining('Invalid os_version range')); }); - test('should throw an error if date range is invalid', () => { + test('should error if date range is invalid', () => { const study = Study.fromJson({ name: 'study', experiment: [ @@ -541,8 +613,8 @@ describe('validateStudy', () => { }, }); - expect(() => { - study_validation.validateStudy(study, studyFilePath); - }).toThrowError('Invalid date range'); + expect( + study_validation.getStudyErrors(study, studyFileBaseName), + ).toContainEqual(expect.stringContaining('Invalid date range')); }); }); diff --git a/src/seed_tools/utils/study_validation.ts b/src/seed_tools/utils/study_validation.ts index d95d17e4..d58b712d 100644 --- a/src/seed_tools/utils/study_validation.ts +++ b/src/seed_tools/utils/study_validation.ts @@ -3,26 +3,14 @@ // License, v. 2.0. If a copy of the MPL was not distributed with this file, // You can obtain one at https://mozilla.org/MPL/2.0/. -import path from 'path'; import { type Study, type Study_Experiment } from '../../proto/generated/study'; import * as study_filter_utils from './study_filter_utils'; const invalidFeatureOrFieldTrialNameChars = ',<*'; const invalidExperimentNameChars = '<*'; -// Validate a study for common errors. Throws an error if any is found. -export function validateStudy(study: Study, studyFilePath: string) { - const errors = validateStudyReturnErrors(study, studyFilePath); - if (errors.length > 0) { - throw new Error(`Error validating ${studyFilePath}:\n${errors.join('\n')}`); - } -} - // Validate a study for common errors. Returns an array of error messages. -export function validateStudyReturnErrors( - study: Study, - studyFilePath: string, -): string[] { +export function getStudyErrors(study: Study, fileBaseName: string): string[] { const errors: string[] = []; const validators = [ checkName, @@ -35,7 +23,7 @@ export function validateStudyReturnErrors( ]; for (const validator of validators) { try { - errors.push(...validator(study, studyFilePath)); + errors.push(...validator(study, fileBaseName)); } catch (e) { if (e instanceof Error) { errors.push(e.message); @@ -49,9 +37,8 @@ export function validateStudyReturnErrors( } // Check that study name matches the file name. -function checkName(study: Study, studyFilePath: string): string[] { +function checkName(study: Study, fileBaseName: string): string[] { const errors: string[] = []; - const fileBaseName = path.basename(studyFilePath, '.json'); if ( study.name !== fileBaseName && !study.name.startsWith(`${fileBaseName}_`) @@ -117,6 +104,16 @@ function checkExperiments(study: Study): string[] { featureNamesToCheck.push(featureAssociations.forcing_feature_off); hasForcingFeatureOff = true; } + // Check featureNamesToCheck is unique. + const featureNamesSet = new Set(featureNamesToCheck); + if (featureNamesSet.size !== featureNamesToCheck.length) { + const duplicateNames = featureNamesToCheck.filter( + (name, index) => featureNamesToCheck.indexOf(name) !== index, + ); + errors.push( + `Duplicate feature name(s) "${duplicateNames.join(', ')}" in feature_association for experiment ${experiment.name}`, + ); + } for (const featureName of featureNamesToCheck) { checkFeatureName(experiment, featureName, errors); }