Skip to content

Don't serve ab tests set to off #14684

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

Merged
Jakeii merged 6 commits into from
Oct 21, 2025
Merged

Don't serve ab tests set to off #14684

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9afb5c5
Don't serve ab tests set to off
Jakeii Oct 15, 2025
c38f8f7
make test more explicit
Jakeii Oct 15, 2025
7e14f40
don't upload inactive tests to fastly
Jakeii Oct 16, 2025
d10f226
remove expiry method
Jakeii Oct 16, 2025
fe93951
metrics
Jakeii Oct 16, 2025
ef2a001
frontend
Jakeii Oct 16, 2025
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 ab-testing/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,10 @@ To add a test where there is not enough space in the default audience space (`A`

For example if there are already 3 25% tests in space `A` totalling 75%, and you want to run a 50% test, you can set the `audienceSpace` to `B` to allow this test to overlap with the existing tests.

### Test Status

Tests can be set to `ON` or `OFF` using the `status` field. Only tests with status `ON` will be validated and deployed.

## How it works

The AB testing framework uses Deno to run scripts that validate and deploy the tests. The `deno.json` file contains the tasks that can be run, such as `validate`, `deploy`, and `build`.
Expand Down
6 changes: 5 additions & 1 deletion ab-testing/abTest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,8 @@ import type { ABTest } from './types';
* - 100% Test variant MVT 500-999
*/

export const ABTests: ABTest[] = [];
const ABTests: ABTest[] = [];

const activeABtests = ABTests.filter((test) => test.status === 'ON');
Copy link
Contributor

@SiAdcock SiAdcock Oct 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking low-priority possibly-daft question for my own curiosity: Should we also filter out tests that have expired at this point?

Copy link
Member Author

@Jakeii Jakeii Oct 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No thats a sensible question! Expired tests are already ignored by Fastly. They also still currently fail the AB test validation (and CI).

Will address stopping them from breaking the validation (as we discussed in the WebEx catchup), separately! Which will likely involve filtering them out here 👍

Copy link
Member Author

@Jakeii Jakeii Oct 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry Monday brain, I added a prompt to the expiration error, https://github.com/guardian/dotcom-rendering/pull/14684/files#diff-4202f37f98d009ed152e65d2bb1a06b05cc758b7b4857bb77628fbdb058b2623L15

The idea is that if you encounter someone else's expired test while adding your own, you can simply set it to off. Hopefully will encourage informing the team!

And we can also hopefully later add a mechanism to alert teams to their expire tests.

Copy link
Contributor

@SiAdcock SiAdcock Oct 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Jakeii, I thought Fastly might be filtering out the expired tests. I guess I was thinking, we're doing some filtering here and some filtering on Fastly... why not do it all here? I haven't thought about it too deeply though.

Copy link
Member Author

@Jakeii Jakeii Oct 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, didn't answer your question did I!

Only on tests are validated, we can't exclude expired tests as we still want them to be validated.

Copy link
Contributor

@SiAdcock SiAdcock Oct 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh yes, that makes sense. So the owners of expired tests can receive some sort of notification that the test has indeed expired. Thanks for connecting the dots for me! 😊


export { ABTests as allABTests, activeABtests };
6 changes: 3 additions & 3 deletions ab-testing/frontend/src/routes/+page.svelte
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<script lang="ts">
import { ABTests } from '../../../abTest';
import { allABTests, activeABtests } from '../../../abTest';
import Table from '$lib/components/TableFixed.svelte';
import AudienceBreakdown from '$lib/components/AudienceBreakdown.svelte';
</script>
Expand Down Expand Up @@ -31,8 +31,8 @@
</p>
</section>
<section>
<AudienceBreakdown tests={ABTests} />
<Table tests={ABTests} />
<AudienceBreakdown tests={activeABtests} />
<Table tests={allABTests} />
</section>

<style>
Expand Down
4 changes: 2 additions & 2 deletions ab-testing/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// @ts-ignore - extension is required to import this as a package in DCR
import { ABTests } from './abTest.ts';
import { ABTests, activeABtests } from './abTest.ts';

export { ABTests };
export { ABTests, activeABtests };
6 changes: 3 additions & 3 deletions ab-testing/scripts/build/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
import { ABTests } from '../../abTest.ts';
import { getMVTGroupsFromDictionary } from '../lib/fastly-api.ts';
import { buildABTestGroupKeyValues } from './build-ab-tests-dict.ts';
import { parseArgs } from 'jsr:@std/cli/parse-args';
import { calculateAllSpaceUpdates } from './calculate-mvt-updates.ts';
import { parseMVTValue, stringifyMVTValue } from '../lib/fastly-subfield.ts';
import { dirname } from 'jsr:@std/path';
import { activeABtests } from '../../abTest.ts';

const flags = parseArgs(Deno.args, {
string: ['mvts', 'ab-tests'],
Expand All @@ -26,9 +26,9 @@ const mvtGroups = new Map(
}),
);

const abTestGroupKeyValues = buildABTestGroupKeyValues(ABTests);
const abTestGroupKeyValues = buildABTestGroupKeyValues(activeABtests);

const mvtIdKeyValues = calculateAllSpaceUpdates(mvtGroups, ABTests);
const mvtIdKeyValues = calculateAllSpaceUpdates(mvtGroups, activeABtests);

const mvtDictArray = Array.from(
mvtIdKeyValues.entries().map(([key, value]) => ({
Expand Down
4 changes: 2 additions & 2 deletions ab-testing/scripts/validation/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { ABTests } from '../../abTest.ts';
import { ABTest } from '../../types.ts';
import { activeABtests } from '../../abTest.ts';
import { enoughSpace } from './enoughSpace.ts';
import { limitServerSideTests } from './limitServerSide.ts';
import { uniqueName } from './uniqueName.ts';
Expand All @@ -19,7 +19,7 @@ function validateTests(testList: ABTest[]) {
}

try {
validateTests(ABTests);
validateTests(activeABtests);
console.log('AB test validations passed');
} catch (err) {
const error = err as Error;
Expand Down
2 changes: 1 addition & 1 deletion ab-testing/scripts/validation/validExpiration.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ export function allExpirationsValid(tests: ABTest[]): boolean {
throw new Error(
`${
test.name
} has an expiration date in the past: ${expires.toISOString()}, has it expired?`,
} has an expiration date in the past: ${expires.toISOString()}, has it expired? If it doesn't belong to you or your team, you can set the status to OFF for now.`,
);
});
}
4 changes: 2 additions & 2 deletions dotcom-rendering/src/components/Metrics.importable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import type { ABTest, ABTestAPI } from '@guardian/ab-core';
import { ABTests } from '@guardian/ab-testing';
import { activeABtests } from '@guardian/ab-testing';
import {
bypassCommercialMetricsSampling,
EventTimer,
Expand Down Expand Up @@ -33,7 +33,7 @@ const clientSideTestsToForceMetrics: ABTest[] = [
];

const shouldCollectMetricsForBetaTests = (userTestParticipations: string[]) => {
const userParticipationConfigs = ABTests.filter((test) =>
const userParticipationConfigs = activeABtests.filter((test) =>
userTestParticipations.includes(test.name),
);
return userParticipationConfigs.some(
Expand Down
Loading