Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Brave-specific restrictions to studies. #1239

Merged
merged 2 commits into from
Oct 24, 2024
Merged

Add Brave-specific restrictions to studies. #1239

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ff718e5
Add Brave-specific restrictions to studies.
goodov Oct 24, 2024
faab5ed
Simplify checks.
goodov Oct 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions src/seed_tools/utils/seed_validation.test.ts
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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