Support legacy seed in perf (#1256)

The PR support `npm run seed_tools create -- --revision
<old_revision_with_seed_json>`

.github/workflows/test-src.yml

@@ -9,6 +9,8 @@ jobs:
     runs-on: ubuntu-latest
     steps:
       - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4
+        with:
+          fetch-depth: '0' # Used to test `seed_tools create --revision <sha1>`
 
       - name: npm install
         run: npm install

src/seed_tools/commands/create.test.ts

@@ -17,15 +17,20 @@ const compareProtobuf = (actual: Uint8Array, expectedFilename: string) => {
   if (fs_sync.existsSync(expectedFilename)) {
     expectedJson = fs_sync.readFileSync(expectedFilename, 'utf-8');
   }
-  const expected = VariationsSeed.fromJsonString(expectedJson);
+  const expected = VariationsSeed.fromJsonString(expectedJson, {
+    ignoreUnknownFields: false,
+  });
   const actualObj = VariationsSeed.fromBinary(actual);
   try {
     expect(actualObj).toEqual(expected);
   } catch (error) {
     const failedFilename = expectedFilename + '.failed';
     fs_sync.writeFileSync(
       failedFilename,
-      VariationsSeed.toJsonString(actualObj, { prettySpaces: 2 }) + '\n',
+      VariationsSeed.toJsonString(actualObj, {
+        useProtoFieldName: true,
+        prettySpaces: 2,
+      }) + '\n',
     );
     throw error;
   }
@@ -111,12 +116,14 @@ describe('create command', () => {
       expect(VariationsSeed.fromBinary(output).version).toEqual('1');
     };
 
-    fs_sync.readdirSync(validSeedsDir).forEach((testCase) => {
-      it(testCase, () => runTest(testCase, undefined));
+    it('test1', () => runTest('test1', undefined));
 
-      // Check creating seed using git history.
-      it(`${testCase}_old_revision`, () => runTest(testCase, 'HEAD'));
-    });
+    // Check creating seed using git history.
+    it('test1_git_revision', () => runTest('test1', 'HEAD'));
+
+    // Check creating seed using git history for legacy seed.json.
+    it('legacy_seed', () =>
+      runTest('legacy_seed', '3f3eb03e12eb7f37a315f66f735d3decb483a90d'));
   });
 
   test('set seed version', async () => {
@@ -203,6 +210,16 @@ describe('create command', () => {
             serialNumberPath,
           ]),
         ).rejects.toThrowError('process.exit(1)');
+
+        const expectedError = (
+          await fs.readFile(
+            path.join(testCaseDir, 'expected_errors.txt'),
+            'utf-8',
+          )
+        ).trim();
+        expect(errorMock).toHaveBeenCalledWith(
+          expect.stringContaining(expectedError),
+        );
       },
     );
   });

src/seed_tools/commands/split_seed_json.ts

@@ -5,9 +5,7 @@
 
 import { Command } from '@commander-js/extra-typings';
 import { promises as fs } from 'fs';
-import DefaultMap from '../../base/containers/default_map';
-import { type Study } from '../../proto/generated/study';
-import { VariationsSeed } from '../../proto/generated/variations_seed';
+import { parseLegacySeedJson } from '../utils/legacy_json_to_seed';
 import * as study_json_utils from '../utils/study_json_utils';
 
 export default function createCommand() {
@@ -19,21 +17,8 @@ export default function createCommand() {
 }
 
 async function main(seedPath: string, outputDir: string) {
-  const seedJson = preprocessSeedJson(
-    JSON.parse(await fs.readFile(seedPath, 'utf8')),
-  );
-
-  // Parse the seed as protobuf json representation. The parse will fail if any
-  // unknown fields or values are present in the json.
-  const parsedSeed = VariationsSeed.fromJson(seedJson, {
-    ignoreUnknownFields: false,
-  });
-
-  const studies = new DefaultMap<string, Study[]>(() => []);
-  for (const study of parsedSeed.study) {
-    studies.get(study.name).push(study);
-  }
-
+  const seedContent = await fs.readFile(seedPath, 'utf8');
+  const { studiesMap } = parseLegacySeedJson(seedContent);
   await fs.mkdir(outputDir, { recursive: true });
 
   // Remove all files in the output directory.
@@ -45,51 +30,10 @@ async function main(seedPath: string, outputDir: string) {
   }
 
   // Write study files.
-  for (const study of studies) {
+  for (const study of studiesMap) {
     const studyName = study[0];
     const studyArray = study[1];
     const studyFile = `${outputDir}/${studyName}.json5`;
     await study_json_utils.writeStudyFile(studyArray, studyFile);
   }
 }
-
-function preprocessSeedJson(json: any): any {
-  json.study = json.studies;
-  delete json.studies;
-
-  for (const study of json.study) {
-    if (study.experiments !== undefined) {
-      study.experiment = study.experiments;
-      delete study.experiments;
-    }
-    for (const experiment of study.experiment) {
-      if (experiment.parameters !== undefined) {
-        experiment.param = experiment.parameters;
-        delete experiment.parameters;
-      }
-    }
-    if (study.filter !== undefined) {
-      if (study.filter.channel !== undefined) {
-        study.filter.channel = study.filter.channel.map((channel: string) => {
-          switch (channel) {
-            case 'NIGHTLY':
-              return 'CANARY';
-            case 'RELEASE':
-              return 'STABLE';
-            default:
-              return channel;
-          }
-        });
-      }
-      if (study.filter.platform !== undefined) {
-        study.filter.platform = study.filter.platform.map(
-          (platform: string) => {
-            return 'PLATFORM_' + platform;
-          },
-        );
-      }
-    }
-  }
-
-  return json;
-}

src/seed_tools/utils/legacy_json_to_seed.ts

@@ -0,0 +1,69 @@
+// 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 DefaultMap from '../../base/containers/default_map';
+import { type Study } from '../../proto/generated/study';
+import { VariationsSeed } from '../../proto/generated/variations_seed';
+
+export function parseLegacySeedJson(seedContent: string): {
+  parsedSeed: VariationsSeed;
+  studiesMap: DefaultMap<string, Study[]>;
+} {
+  const seedJson = preprocessSeedJson(JSON.parse(seedContent));
+
+  // Parse the seed as protobuf json representation. The parse will fail if any
+  // unknown fields or values are present in the json.
+  const parsedSeed = VariationsSeed.fromJson(seedJson, {
+    ignoreUnknownFields: false,
+  });
+
+  const studiesMap = new DefaultMap<string, Study[]>(() => []);
+  for (const study of parsedSeed.study) {
+    studiesMap.get(study.name).push(study);
+  }
+
+  return { parsedSeed, studiesMap };
+}
+
+function preprocessSeedJson(json: any): any {
+  json.study = json.studies;
+  delete json.studies;
+
+  for (const study of json.study) {
+    if (study.experiments !== undefined) {
+      study.experiment = study.experiments;
+      delete study.experiments;
+    }
+    for (const experiment of study.experiment) {
+      if (experiment.parameters !== undefined) {
+        experiment.param = experiment.parameters;
+        delete experiment.parameters;
+      }
+    }
+    if (study.filter !== undefined) {
+      if (study.filter.channel !== undefined) {
+        study.filter.channel = study.filter.channel.map((channel: string) => {
+          switch (channel) {
+            case 'NIGHTLY':
+              return 'CANARY';
+            case 'RELEASE':
+              return 'STABLE';
+            default:
+              return channel;
+          }
+        });
+      }
+      if (study.filter.platform !== undefined) {
+        study.filter.platform = study.filter.platform.map(
+          (platform: string) => {
+            return 'PLATFORM_' + platform;
+          },
+        );
+      }
+    }
+  }
+
+  return json;
+}

src/seed_tools/utils/studies_to_seed.ts

@@ -17,6 +17,8 @@ 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';
+import { parseLegacySeedJson } from './legacy_json_to_seed';
+import { validateName } from './study_validation';
 
 export async function readStudiesToSeed(
   studiesDir: string,
@@ -71,20 +73,44 @@ async function readStudiesAtRevision(
 }> {
   const basePath = wsPath('//');
   studiesDir = path.relative(basePath, studiesDir);
-  const files = execSync(`git show "${revision}":"${studiesDir}"`, {
-    encoding: 'utf8',
-  }).split('\n');
 
-  const filesWithContent = [];
-  for (const file of files) {
-    if (!file.endsWith('.json5')) continue;
-    const content = execSync(`git show ${revision}:"${studiesDir}/${file}"`, {
+  // Validate revision format.
+  if (!/^[a-z0-9]+$/.test(revision) && revision !== 'HEAD') {
+    return {
+      studies: [],
+      studyFileBaseNameMap: new Map(),
+      errors: [`Invalid revision: ${revision}`],
+    };
+  }
+
+  try {
+    const files = execSync(`git show "${revision}":"${studiesDir}"`, {
+      encoding: 'utf8',
+    }).split('\n');
+
+    const filesWithContent = [];
+    for (const file of files) {
+      if (!file.endsWith('.json5')) continue;
+      if (!validateName(file, 'filename', [])) continue;
+
+      const content = execSync(`git show ${revision}:"${studiesDir}/${file}"`, {
+        encoding: 'utf8',
+      });
+      filesWithContent.push({ path: file, content });
+    }
+    return await readStudies(filesWithContent, false);
+  } catch {
+    console.log(`Failed to read studies ${revision}, use seed.json fallback:`);
+    const seedContent = execSync(`git show "${revision}":seed/seed.json`, {
       encoding: 'utf8',
     });
-    filesWithContent.push({ path: file, content });
+    const { parsedSeed } = parseLegacySeedJson(seedContent);
+    return {
+      studies: parsedSeed.study,
+      studyFileBaseNameMap: new Map(),
+      errors: [],
+    };
   }
-
-  return await readStudies(filesWithContent, false);
 }
 
 async function readStudies(

src/seed_tools/utils/study_validation.test.ts

@@ -110,7 +110,7 @@ describe('getStudyErrors', () => {
     );
   });
 
-  test.each(['exp😀', 'exp<', 'exp*'])(
+  test.each(['exp😀', 'exp<', 'exp*', 'exp,'])(
     'should error if experiment name has invalid char %s',
     (experimentName) => {
       const study = Study.fromJson({
@@ -131,26 +131,6 @@ describe('getStudyErrors', () => {
     },
   );
 
-  test('should not error if experiment name has comma', () => {
-    const study = Study.fromJson({
-      name: 'study',
-      experiment: [
-        {
-          name: 'experiment1,',
-          probability_weight: 100,
-        },
-      ],
-      filter: {
-        platform: ['PLATFORM_LINUX'],
-        channel: ['BETA'],
-      },
-    });
-
-    expect(study_validation.getStudyErrors(study, studyFileBaseName)).toEqual(
-      [],
-    );
-  });
-
   test('should error if duplicate experiment names are found', () => {
     const study = Study.fromJson({
       name: 'study',
@@ -225,7 +205,7 @@ describe('getStudyErrors', () => {
           study_validation.getStudyErrors(study, studyFileBaseName),
         ).toContainEqual(
           expect.stringContaining(
-            `Invalid feature name for experiment experiment: ${featureName}`,
+            `Invalid feature name: ${featureName} (use only 0-9,a-z,A-Z,_,-)`,
           ),
         );
       }