Update version tests and parsing to show clearer error message.

src/seed_tools/utils/seed_validation.ts

@@ -19,7 +19,21 @@ export function validateSeed(seed: VariationsSeed) {
 
 // Validate a seed for common errors. Returns an array of error messages.
 export function validateSeedReturnErrors(seed: VariationsSeed): string[] {
-  return checkOverlappingStudies(seed);
+  const errors: string[] = [];
+  const validators = [checkOverlappingStudies];
+  for (const validator of validators) {
+    try {
+      errors.push(...validator(seed));
+    } catch (e) {
+      if (e instanceof Error) {
+        errors.push(e.message);
+      } else {
+        // Rethrow non-Error exceptions.
+        throw e;
+      }
+    }
+  }
+  return errors;
 }
 
 // Validates a seed by checking for overlapping studies that use the same

src/seed_tools/utils/study_json_utils.ts

@@ -36,7 +36,7 @@ export async function readStudyFileReturnWithError(
       e.message += ` (${studyFilePath})`;
       return Result.error(e);
     }
-    // Re-throw non-Error exceptions.
+    // Rethrow non-Error exceptions.
     throw e;
   }
 }

src/seed_tools/utils/study_validation.test.ts

@@ -178,6 +178,25 @@ describe('validateStudy', () => {
     }).toThrowError('Invalid version range');
   });
 
+  test('should throw an error if version is invalid', () => {
+    const study = Study.fromJson({
+      name: 'study',
+      experiment: [
+        {
+          name: 'experiment1',
+          probability_weight: 100,
+        },
+      ],
+      filter: {
+        min_version: '2.a',
+      },
+    });
+
+    expect(() => {
+      study_validation.validateStudy(study, studyFilePath);
+    }).toThrowError('contains non-numeric characters');
+  });
+
   test('should throw an error if os version range is invalid', () => {
     const study = Study.fromJson({
       name: 'study',

src/seed_tools/utils/study_validation.ts

@@ -20,14 +20,28 @@ export function validateStudyReturnErrors(
   study: Study,
   studyFilePath: string,
 ): string[] {
-  return Array.prototype.concat(
-    checkName(study, studyFilePath),
-    checkExperiments(study),
-    checkFilterExcludeFields(study),
-    checkDateRange(study),
-    checkVersionRange(study),
-    checkOsVersionRange(study),
-  );
+  const errors: string[] = [];
+  const validators = [
+    checkName,
+    checkExperiments,
+    checkFilterExcludeFields,
+    checkDateRange,
+    checkVersionRange,
+    checkOsVersionRange,
+  ];
+  for (const validator of validators) {
+    try {
+      errors.push(...validator(study, studyFilePath));
+    } catch (e) {
+      if (e instanceof Error) {
+        errors.push(e.message);
+      } else {
+        // Rethrow non-Error exceptions.
+        throw e;
+      }
+    }
+  }
+  return errors;
 }
 
 // Check that study name matches the file name.

src/seed_tools/utils/version.test.ts

@@ -8,82 +8,83 @@ import { Version } from './version';
 describe('Version', () => {
   describe('parse', () => {
     const testCases = [
-      { input: '', parts: 0, firstpart: 0, success: false },
-      { input: ' ', parts: 0, firstpart: 0, success: false },
-      { input: '\t', parts: 0, firstpart: 0, success: false },
-      { input: '\n', parts: 0, firstpart: 0, success: false },
-      { input: '  ', parts: 0, firstpart: 0, success: false },
-      { input: '.', parts: 0, firstpart: 0, success: false },
-      { input: ' . ', parts: 0, firstpart: 0, success: false },
-      { input: '0', parts: 1, firstpart: 0, success: true },
-      { input: '0.', parts: 0, firstpart: 0, success: false },
-      { input: '0.0', parts: 2, firstpart: 0, success: true },
+      { input: '', components: [] },
+      { input: ' ', components: [] },
+      { input: '\t', components: [] },
+      { input: '\n', components: [] },
+      { input: '  ', components: [] },
+      { input: '.', components: [] },
+      { input: ' . ', components: [] },
+      { input: '0', components: [0] },
+      { input: '0.', components: [] },
+      { input: '0.0', components: [0, 0] },
       {
         input: '4294967295.0',
-        parts: 2,
-        firstpart: 4294967295,
-        success: true,
+        components: [4294967295, 0],
       },
-      { input: '4294967296.0', parts: 0, firstpart: 0, success: false },
-      { input: '-1.0', parts: 0, firstpart: 0, success: false },
-      { input: '1.-1.0', parts: 0, firstpart: 0, success: false },
-      { input: '1,--1.0', parts: 0, firstpart: 0, success: false },
-      { input: '+1.0', parts: 0, firstpart: 0, success: false },
-      { input: '1.+1.0', parts: 0, firstpart: 0, success: false },
-      { input: '1+1.0', parts: 0, firstpart: 0, success: false },
-      { input: '++1.0', parts: 0, firstpart: 0, success: false },
-      { input: '1.0a', parts: 0, firstpart: 0, success: false },
+      { input: '4294967296.0', components: [] },
+      { input: '-1.0', components: [] },
+      { input: '1.-1.0', components: [] },
+      { input: '1,--1.0', components: [] },
+      { input: '+1.0', components: [] },
+      { input: '1.+1.0', components: [] },
+      { input: '1+1.0', components: [] },
+      { input: '++1.0', components: [] },
+      { input: '1.0a', components: [] },
       {
         input: '1.2.3.4.5.6.7.8.9.0',
-        parts: 10,
-        firstpart: 1,
-        success: true,
+        components: [1, 2, 3, 4, 5, 6, 7, 8, 9, 0],
       },
-      { input: '02.1', parts: 0, firstpart: 0, success: false },
-      { input: '0.01', parts: 2, firstpart: 0, success: true },
-      { input: 'f.1', parts: 0, firstpart: 0, success: false },
-      { input: '15.007.20011', parts: 3, firstpart: 15, success: true },
-      { input: '15.5.28.130162', parts: 4, firstpart: 15, success: true },
+      { input: '02.1', components: [] },
+      { input: '0.01', components: [0, 1] },
+      { input: 'f.1', components: [] },
+      { input: '15.007.20011', components: [15, 7, 20011] },
+      { input: '15.5.28.130162', components: [15, 5, 28, 130162] },
     ];
 
     testCases.forEach((testCase) => {
-      it(`should ${testCase.success ? 'parse' : 'fail'} "${testCase.input}"`, () => {
-        if (!testCase.success) {
-          expect(() => new Version(testCase.input)).toThrowError();
-        } else {
+      it(`should ${testCase.components.length > 0 ? 'parse' : 'fail'} "${testCase.input}"`, () => {
+        if (testCase.components.length > 0) {
           const version = new Version(testCase.input);
           expect(
             () => new Version(testCase.input, { disallowWildcard: true }),
           ).not.toThrow();
-          expect(version.components.length).toBe(testCase.parts);
-          expect(version.components[0]).toBe(testCase.firstpart);
+          expect(version.components).toStrictEqual(testCase.components);
           expect(version.isWildcard).toBe(false);
+        } else {
+          expect(() => new Version(testCase.input)).toThrowError();
         }
       });
     });
 
     describe('wildcard', () => {
       const testCases = [
-        { version: '1.2.3.*', expected: true, isWildcard: true },
-        { version: '1.2.3.5*', expected: false, isWildcard: false },
-        { version: '1.2.3.56*', expected: false, isWildcard: false },
-        { version: '1.*.3', expected: false, isWildcard: false },
-        { version: '20.*', expected: true, isWildcard: true },
-        { version: '+2.*', expected: false, isWildcard: false },
-        { version: '*', expected: false, isWildcard: false },
-        { version: '*.2', expected: false, isWildcard: true },
+        { version: '1.2.3.*', components: [1, 2, 3] },
+        { version: '1.2.3.5*', components: [] },
+        { version: '1.2.3.56*', components: [] },
+        { version: '1.2.3.56.a.*', components: [] },
+        { version: '1.2.3.56.*.a', components: [] },
+        { version: '1.*.3', components: [] },
+        { version: '20.*', components: [20] },
+        { version: '+2.*', components: [] },
+        { version: '*', components: [] },
+        { version: '*.2', components: [] },
       ];
 
       testCases.forEach((testCase) => {
-        it(`should ${testCase.expected ? 'parse' : 'fail'} "${testCase.version}"`, () => {
-          if (testCase.expected) {
+        it(`should ${testCase.components.length > 0 ? 'parse' : 'fail'} "${testCase.version}"`, () => {
+          if (testCase.components.length > 0) {
             const version = new Version(testCase.version);
-            expect(version.isWildcard).toBe(testCase.isWildcard);
+            expect(version.components).toStrictEqual(testCase.components);
+            expect(version.isWildcard).toBe(true);
             expect(
               () => new Version(testCase.version, { disallowWildcard: true }),
             ).toThrowError();
           } else {
             expect(() => new Version(testCase.version)).toThrowError();
+            expect(
+              () => new Version(testCase.version, { disallowWildcard: true }),
+            ).toThrowError();
           }
         });
       });

src/seed_tools/utils/version.ts

@@ -3,6 +3,8 @@
 // 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 Result from '../../base/result';
+
 export interface VersionOptions {
   disallowLeadingZeros?: boolean;
   disallowWildcard?: boolean;
@@ -46,8 +48,12 @@ export class Version {
         continue;
       }
 
-      const parsedComponent = parseVersionComponent(strComponent);
+      const parsedComponentResult = parseVersionComponent(strComponent);
+      if (!parsedComponentResult.ok) {
+        throw new Error(`${parsedComponentResult.error}: ${versionStr}`);
+      }
 
+      const parsedComponent = parsedComponentResult.value;
       // base::Version allows leading zeros in subsequent parts (but not the
       // first) for legacy reasons. This is not necessary for our use case, so
       // we can forbid them.
@@ -121,18 +127,18 @@ export class Version {
 
 // Strictly parse a string as a number that can be represented as uint32. This
 // is required to match the behavior of base::Version in C++.
-function parseVersionComponent(str: string): number {
+function parseVersionComponent(str: string): Result<number, string> {
   if (!/^\d+$/.test(str)) {
-    throw new Error(
-      `Version component contains non-numeric characters: ${str}`,
+    return Result.error(
+      `Version component "${str}" contains non-numeric characters`,
     );
   }
 
   const num = parseInt(str, 10);
 
   if (num < 0 || num > 4294967295) {
-    throw new Error(`Version component does not fit into uint32: ${str}`);
+    return Result.error(`Version component "${str}" does not fit into uint32`);
   }
 
-  return num;
+  return Result.ok(num);
 }