Skip to content

Commit

Permalink
Add Brave-specific restrictions to studies. (#1239)
Browse files Browse the repository at this point in the history 
Add few restrictions to studies:
* Channel and platform should be set (we have this in Python generator).
* Ensure the version is Brave-like version.
  • Loading branch information
@goodov
goodov authored Oct 24, 2024
1 parent 95d72c3 commit 7252645
Show file tree
Hide file tree
Showing 3 changed files with 153 additions and 0 deletions.
4 changes: 4 additions & 0 deletions src/seed_tools/utils/seed_validation.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,11 @@ describe('getSeedErrors', () => {
};
if (filter !== null) {
study.filter = filter;
} else {
study.filter = {};
}
study.filter.platform = study.filter.platform ?? ['PLATFORM_LINUX'];
study.filter.channel = study.filter.channel ?? ['BETA'];
return Study.fromJson(study, {
ignoreUnknownFields: false,
});
Expand Down
101 changes: 101 additions & 0 deletions src/seed_tools/utils/study_validation.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -140,6 +140,10 @@ describe('getStudyErrors', () => {
probability_weight: 100,
},
],
filter: {
platform: ['PLATFORM_LINUX'],
channel: ['BETA'],
},
});

expect(study_validation.getStudyErrors(study, studyFileBaseName)).toEqual(
Expand Down Expand Up @@ -258,6 +262,10 @@ describe('getStudyErrors', () => {
forcing_flag: 'hello',
},
],
filter: {
platform: ['PLATFORM_LINUX'],
channel: ['BETA'],
},
});

expect(study_validation.getStudyErrors(study, studyFileBaseName)).toEqual(
Expand All @@ -277,6 +285,10 @@ describe('getStudyErrors', () => {
forcing_flag: forcingFlag,
},
],
filter: {
platform: ['PLATFORM_LINUX'],
channel: ['BETA'],
},
});

expect(
Expand Down Expand Up @@ -349,6 +361,10 @@ describe('getStudyErrors', () => {
feature_association: {},
},
],
filter: {
platform: ['PLATFORM_LINUX'],
channel: ['BETA'],
},
};
if (forcingFeatureOn) {
(
Expand Down Expand Up @@ -527,6 +543,8 @@ describe('getStudyErrors', () => {
},
],
filter: {
platform: ['PLATFORM_LINUX'],
channel: ['BETA'],
locale: [],
exclude_locale: ['en'],
},
Expand Down Expand Up @@ -578,6 +596,48 @@ describe('getStudyErrors', () => {
);
});

test.each([{ min_version: '130.0.6517.0' }, { max_version: '135.0.6707.0' }])(
'should error if version is Chromium %s',
(filter: any) => {
const study = Study.fromJson({
name: 'study',
experiment: [
{
name: 'experiment1',
probability_weight: 100,
},
],
filter,
});

expect(
study_validation.getStudyErrors(study, studyFileBaseName),
).toContainEqual(
expect.stringContaining('Detected Chromium version in a filter'),
);
},
);

test.each([{ min_version: '130.1.70.0' }, { max_version: '135.1.91.0' }])(
'should not error if version is Brave %s',
(filter: any) => {
const study = Study.fromJson({
name: 'study',
experiment: [
{
name: 'experiment1',
probability_weight: 100,
},
],
filter: { ...filter, platform: ['PLATFORM_LINUX'], channel: ['BETA'] },
});

expect(study_validation.getStudyErrors(study, studyFileBaseName)).toEqual(
[],
);
},
);

test('should error if os version range is invalid', () => {
const study = Study.fromJson({
name: 'study',
Expand Down Expand Up @@ -617,4 +677,45 @@ describe('getStudyErrors', () => {
study_validation.getStudyErrors(study, studyFileBaseName),
).toContainEqual(expect.stringContaining('Invalid date range'));
});

test.each([{}, { channel: [] }, { channel: ['BETA', 'BETA'] }])(
'should error if channel is invalid',
(filter: any) => {
const study = Study.fromJson({
name: 'study',
experiment: [
{
name: 'experiment1',
probability_weight: 100,
},
],
filter: { ...filter, platform: ['PLATFORM_LINUX'] },
});

expect(
study_validation.getStudyErrors(study, studyFileBaseName),
).toContainEqual(expect.stringMatching(/(C|c)hannel/));
},
);

test.each([
{},
{ platform: [] },
{ platform: ['PLATFORM_LINUX', 'PLATFORM_LINUX'] },
])('should error if platform is invalid', (filter: any) => {
const study = Study.fromJson({
name: 'study',
experiment: [
{
name: 'experiment1',
probability_weight: 100,
},
],
filter: { ...filter, channel: ['BETA'] },
});

expect(
study_validation.getStudyErrors(study, studyFileBaseName),
).toContainEqual(expect.stringMatching(/(P|p)latform/));
});
});
48 changes: 48 additions & 0 deletions src/seed_tools/utils/study_validation.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

import { type Study, type Study_Experiment } from '../../proto/generated/study';
import * as study_filter_utils from './study_filter_utils';
import { Version } from './version';

const invalidFeatureOrFieldTrialNameChars = ',<*';
const invalidExperimentNameChars = '<*';
Expand All @@ -20,6 +21,7 @@ export function getStudyErrors(study: Study, fileBaseName: string): string[] {
checkDateRange,
checkVersionRange,
checkOsVersionRange,
checkChannelAndPlatform,
];
for (const validator of validators) {
try {
Expand Down Expand Up @@ -233,6 +235,21 @@ function checkVersionRange(study: Study): string[] {
const [minVersion, maxVersion] =
study_filter_utils.getStudyVersionRange(study);

const checkBraveVersionFormat = (version?: Version) => {
if (
version !== undefined &&
version.components.length >= 3 &&
version.components[2] > 6000
) {
errors.push(
`Detected Chromium version in a filter for study ${study.name}: ${version.toString()}. Use Brave version in a format CHROMIUM_MAJOR.BRAVE_MAJOR.BRAVE_MINOR.BRAVE_BUILD`,
);
}
};

checkBraveVersionFormat(minVersion);
checkBraveVersionFormat(maxVersion);

if (
minVersion !== undefined &&
maxVersion !== undefined &&
Expand All @@ -242,6 +259,7 @@ function checkVersionRange(study: Study): string[] {
`Invalid version range for study ${study.name}: min (${minVersion.toString()}) > max (${maxVersion.toString()})`,
);
}

return errors;
}

Expand All @@ -263,6 +281,36 @@ function checkOsVersionRange(study: Study): string[] {
return errors;
}

function checkChannelAndPlatform(study: Study): string[] {
const errors: string[] = [];
if (study.filter === undefined) {
errors.push(`Filter is not defined for study ${study.name}.`);
return errors;
}

const hasDuplicates = (a: any[]) => a.length !== new Set(a).size;

if ((study.filter.channel?.length ?? 0) === 0) {
errors.push(`Channel filter is required for study ${study.name}`);
} else {
// Check if duplicate channels are present.
if (hasDuplicates(study.filter.channel)) {
errors.push(`Duplicate channel(s) in filter for study ${study.name}`);
}
}

if ((study.filter.platform?.length ?? 0) === 0) {
errors.push(`Platform filter is required for study ${study.name}`);
} else {
// Check if duplicate platforms are present.
if (hasDuplicates(study.filter.platform)) {
errors.push(`Duplicate platform(s) in filter for study ${study.name}`);
}
}

return errors;
}

function checkExperimentName(
study: Study,
experimentName: string,
Expand Down

0 comments on commit 7252645

Please sign in to comment.