Bug 1914647 - Using browser tests to collect JSON for live messages #370

sarahhjchung · 2024-09-03T16:19:32Z

Bug 1914647

Changes made:

Added live message data for releases 123-129
Added a script to merge all the release JSON data
Documented steps on how to run browser tests and commit JSON data into Skylight in CONTRIBUTING.md

Things to consider for future patches:

How to handle minor release versions
How to handle versions 115-123 and versions after 129
How to handle property collisions (this can a part of this ticket)
I'll also be rebasing this branch against the spike PR [WIP] Bug 1906939 - Complete list of live messages #346 to compare the live message table between the two PRs

netlify · 2024-09-03T16:19:50Z

✅ Deploy Preview for fxms-skylight ready!

Name	Link
🔨 Latest commit	`ddfde3e`
🔍 Latest deploy log	https://app.netlify.com/sites/fxms-skylight/deploys/66e84a045b9ff700088049ce
😎 Deploy Preview	https://deploy-preview-370--fxms-skylight.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 100 (🟢 up 11 from production) Accessibility: 97 (🟢 up 8 from production) Best Practices: 92 (🟢 up 9 from production) SEO: 89 (🟢 up 7 from production) PWA: - View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

dmose

This looks good to me. r=dmose, once this import script aborts if it sees that the keys aren't in reverse order.

dmose · 2024-09-11T19:03:05Z

lib/mergeASRouterData.js

+  // Add any message data with an id that does not already exist in data.json
+  for (let i = 0; i < json_data.length; i++) {
+    if (!json_result.find((x) => x.id === json_data[i].id)) {
+      json_result.push(json_data[i]);


the first version read always wins (which is fine if releases stay in decreasing order) - abort script if wrong order, comment near this code that it depends on this ordering

the preview will always be of the latest released version, even though metrics will cover multiple (possibly all) versions (future epic)

will need similar algorithm for merge looker data with this data and eventually remote settings data (can be part of spike PR - make note in spike PR)

Instead of aborting script, I just ended up sorting the list in descending order

Please add a comment that this will need to be checked once if/when we add dot-versions (129.0.1), since I'm not sure how sort's default collation order will handle that...

I also had to update the script to start with a basically empty data.json (including the default about welcome message that we need to manually add) to account for when newer releases are being added so that they are prioritized first and not being overwritten.

dmose · 2024-09-11T19:19:15Z

Before you land, could you add in 130 as well, since it's the current release version?

I'd also be interested in your thoughts on how we keep this up-to-date. Might just be a repeating calendar entry or something...

sarahhjchung · 2024-09-11T20:05:08Z

Yeah, I feel like a repeating calendar entry for every release makes the most sense

dmose · 2024-09-11T21:28:54Z

lib/mergeASRouterData.js

+});
+console.log(availableReleases);
+
+const manualMessages = [


Can you clarify why this needs to be manual, and how we go about updating it with each new release? My suspicion is that even if we need to do some kind of hack to get the data, that hack probably wants to go in the browser test itself, so that we always get the current version.

Just talked to @AllegroFox and we figured out how to get the about welcome default message data into the browser test! I'll be pushing my changes soon

I've rebased my new changes to the browser test and you can see them here https://phabricator.services.mozilla.com/D201646

dmose

OK, looks good, once the noted changes have been made. r=dmose

I've got some stuff in my calendar for early October to see if we can automate ticket creation for this with JIRA. I'll either do that in October, or fall back to manual calendaring.

dmose · 2024-09-13T22:50:56Z

lib/mergeASRouterData.js

+  JSON.stringify([]),
+);
+
+availableReleases.map((release) => {


In the interest of making the code easier to read, how about changing this from an anonymous lambda function to a named function with comments describing the expected behavior?

dmose · 2024-09-13T23:13:55Z

CONTRIBUTING.md

+   mv /tmp/file.json ./{VERSION_NUM}-release.json
+   ```
+
+6. Run [`lib/mergeASRouterData.js`](/lib/mergeASRouterData.js) using node to merge all the release data into one file. Make sure you include the latest version number you've just collected data from into the `availableReleases` array inside the script.


Please split this into two steps, with the first being to edit the script and add the version number, and the second step to run the script.

dmose · 2024-09-13T23:14:27Z

CONTRIBUTING.md

+
+6. Run [`lib/mergeASRouterData.js`](/lib/mergeASRouterData.js) using node to merge all the release data into one file. Make sure you include the latest version number you've just collected data from into the `availableReleases` array inside the script.
+
+   **❗Important note**: Make sure to update the template for the `MR_WELCOME_DEFAULT` message inside `data.json` to "defaultaboutwelcome". This is currently a manual step we must perform to ensure that the message preview for this unique message is working.


This line can be deleted now, correct?

sarahhjchung added 3 commits August 30, 2024 10:30

WIP

a2c4aa1

Add message jsons

e89798a

Add docs for browser tests

aa64fe4

sarahhjchung added 3 commits September 3, 2024 12:31

Linting

4f7c69d

Add merge script

907a01d

Fix step in CONTRIBUTING

84a363f

sarahhjchung changed the title ~~[WIP] Bug 1914647 - Document steps on running browser tests to collect JSON for live messages~~ [WIP] Bug 1914647 - Using browser tests to collect JSON for live messages Sep 4, 2024

sarahhjchung added 4 commits September 4, 2024 10:05

Add version 125 data

ee9aba1

Linting

c692ecf

Add version 123 data

97eb473

Linting

8aa5fb9

sarahhjchung changed the title ~~[WIP] Bug 1914647 - Using browser tests to collect JSON for live messages~~ Bug 1914647 - Using browser tests to collect JSON for live messages Sep 4, 2024

sarahhjchung marked this pull request as ready for review September 4, 2024 20:32

sarahhjchung requested a review from dmose September 4, 2024 20:32

dmose approved these changes Sep 11, 2024

View reviewed changes

sarahhjchung mentioned this pull request Sep 11, 2024

[WIP] Bug 1906939 - Complete list of live messages #346

Closed

sarahhjchung added 2 commits September 11, 2024 16:18

Add release 130 and update script

f3ae5bf

linting

1b19195

dmose reviewed Sep 11, 2024

View reviewed changes

sarahhjchung added 2 commits September 12, 2024 13:23

Update with about welcome default changes

a9cc08d

Linting

ffcdf27

dmose approved these changes Sep 13, 2024

View reviewed changes

sarahhjchung added 2 commits September 16, 2024 10:53

Address PR comments

910eade

Add defaultaboutwelcome step

ddfde3e

sarahhjchung merged commit 41ac399 into main Sep 16, 2024
4 checks passed

sarahhjchung deleted the browser-tests branch September 16, 2024 15:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1914647 - Using browser tests to collect JSON for live messages #370

Bug 1914647 - Using browser tests to collect JSON for live messages #370

sarahhjchung commented Sep 3, 2024 •

edited

Loading

netlify bot commented Sep 3, 2024 •

edited

Loading

dmose left a comment

dmose Sep 11, 2024

sarahhjchung Sep 11, 2024

dmose Sep 11, 2024

sarahhjchung Sep 11, 2024

dmose commented Sep 11, 2024

sarahhjchung commented Sep 11, 2024

dmose Sep 11, 2024

sarahhjchung Sep 12, 2024

sarahhjchung Sep 12, 2024

dmose left a comment

dmose Sep 13, 2024

dmose Sep 13, 2024

dmose Sep 13, 2024


		6. Run [`lib/mergeASRouterData.js`](/lib/mergeASRouterData.js) using node to merge all the release data into one file. Make sure you include the latest version number you've just collected data from into the `availableReleases` array inside the script.

		❗Important note: Make sure to update the template for the `MR_WELCOME_DEFAULT` message inside `data.json` to "defaultaboutwelcome". This is currently a manual step we must perform to ensure that the message preview for this unique message is working.

Bug 1914647 - Using browser tests to collect JSON for live messages #370

Bug 1914647 - Using browser tests to collect JSON for live messages #370

Conversation

sarahhjchung commented Sep 3, 2024 • edited Loading

netlify bot commented Sep 3, 2024 • edited Loading

✅ Deploy Preview for fxms-skylight ready!

dmose left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmose commented Sep 11, 2024

sarahhjchung commented Sep 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmose left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sarahhjchung commented Sep 3, 2024 •

edited

Loading

netlify bot commented Sep 3, 2024 •

edited

Loading