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

Update seed testing instruction. #1129

Merged
merged 1 commit into from
Jul 19, 2024
Merged

Update seed testing instruction. #1129

merged 1 commit into from
Jul 19, 2024

Conversation

goodov
Copy link
Member

@goodov goodov commented Jul 17, 2024

No description provided.

Copy link
Contributor

github-actions bot commented Jul 17, 2024

✅ Test Seed Generated Successfully

To apply the test seed:

  1. Desktop: Launch the browser with --variations-pr=1129.
    Android: Set the command line to --variations-pr=1129 in debug menu, restart the browser.
    iOS: Set Variations PR to 1129 in Brave Core Switches debug menu, restart the browser.
  2. Wait 5-10 seconds to fetch the seed.
  3. Restart the browser to apply the seed.
  4. Ensure Active Variations section at brave://version starts with the expected seed version (see below).

Seed Details

  • Version: pull/1129@2c553a195cab4a3cec670511411f9bc8bbdf9ecf
  • Uploaded: 2024-07-19T06:16:44.578Z
  • Serial Number: fd4febf939631858f63e85c905993215

@goodov goodov force-pushed the variations-pr-cmdline branch 6 times, most recently from 11d4e26 to 14b9689 Compare July 17, 2024 15:17
@goodov
Copy link
Member Author

goodov commented Jul 18, 2024

cc @brave/qa-team

can you please proof-read the instruction about testing the seed and improve it if possible (see the comment in this pull request)? Maybe you have a better naming internally for "the debug menu" on android/ios respectively.

seed/seed.json Outdated
@@ -2948,7 +2948,7 @@
"Playlist"
]
},
"name": "Enabled",
"name": "Enabledd",

Choose a reason for hiding this comment

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

What's this about? Is it an intentional change, or a typo?

The rest looks good to me for desktop 👍

Copy link
Member Author

@goodov goodov Jul 19, 2024

Choose a reason for hiding this comment

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

it's a test change to trigger the seed generation (I'm not going to commit it).

Copy link
Member

@kjozwiak kjozwiak left a comment

Choose a reason for hiding this comment

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

LGTM other than the issue that @stephendonner pointed out re: Enabledd. Instructions should be good enough. At this point, most of QA should be able to figure out how to verify using the new method but I'll point them to our verifications via brave/brave-core#24604 (comment) which should have enough context/information.

@goodov
Copy link
Member Author

goodov commented Jul 19, 2024

LGTM other than the issue that @stephendonner pointed out re: Enabledd. Instructions should be good enough. At this point, most of QA should be able to figure out how to verify using the new method but I'll point them to our verifications via brave/brave-core#24604 (comment) which should have enough context/information.

The point here is so any developer who sees brave-variations for the first time can figure this on their own, it's not only about QA.

@goodov goodov marked this pull request as ready for review July 19, 2024 06:18
@goodov goodov merged commit f4b2190 into main Jul 19, 2024
7 checks passed
@goodov goodov deleted the variations-pr-cmdline branch July 19, 2024 06:20
@kjozwiak
Copy link
Member

LGTM other than the issue that @stephendonner pointed out re: Enabledd. Instructions should be good enough. At this point, most of QA should be able to figure out how to verify using the new method but I'll point them to our verifications via brave/brave-core#24604 (comment) which should have enough context/information.

The point here is so any developer who sees brave-variations for the first time can figure this on their own, it's not only about QA.

yup, makes sense 👍

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.

3 participants