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.

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

Fix persisting of forced ab test to localstorage #1658

Merged
merged 3 commits into from
Nov 12, 2024

Conversation

Jakeii
Copy link
Member

@Jakeii Jakeii commented Nov 12, 2024

What does this change?

This used to work, evidenced by the fact that ab-localstorage.ts already existed, and broke at some point.

I'm not sure how leaving the test used to work but have modified slightly to allow leaving by clearing the variant e.g. http://localhost:3030/Front/https://www.theguardian.com/uk#ab-yourTest=

Why?

It's quite useful for the test to persist between pageviews.

@Jakeii Jakeii marked this pull request as ready for review November 12, 2024 16:45
@Jakeii Jakeii requested a review from a team as a code owner November 12, 2024 16:45
Copy link
Contributor

Ad load time test results

For consented, top-above-nav took on average 5161ms to load.
For consentless, top-above-nav took on average 2997ms to load.

Test conditions:

  • 5mbps download speed
  • 1.5mbps upload speed
  • 150ms latency

Copy link
Contributor

@emma-imber emma-imber left a comment

Choose a reason for hiding this comment

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

Nice! This will come in handy for sure

1. Create a test in [src/experiments/tests](https://github.com/guardian/commercial-core/blob/main/src/experiments/tests)
1. Add the test to [concurrent tests](https://github.com/guardian/commercial-core/blob/main/src/experiments/ab-tests.ts)
1. Follow steps 1-6 in [the DCR documentation](https://github.com/guardian/dotcom-rendering/blob/main/dotcom-rendering/docs/development/ab-testing-in-dcr.md)
1. Create a test in [src/experiments/tests](https://github.com/guardian/commercial/blob/main/src/experiments/tests)
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for updating the docs! 🎉

@Jakeii Jakeii merged commit 9c0e78d into main Nov 12, 2024
13 checks passed
@Jakeii Jakeii deleted the jlkl/fix-persisting-forced-ab-localstorage branch November 12, 2024 17:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants