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

Fxp 3678 update browsers on awfy #532

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

Carla-Moz
Copy link

This PR will replace Update browsers fix #530
Closes Jira issue Update browsers on AWFY FXP 3678

Note: For this review, I need help with test/config.test.js li14 test

remove geckoview
remove safari
remove chrome-m and add chromium-as-release
rename Fenix label to Firefox
remove chrome desktop test
added linting
Copy link

netlify bot commented Dec 4, 2024

Deploy Preview for firefox-performance-dashboard ready!

Name Link
🔨 Latest commit 75963d7
🔍 Latest deploy log https://app.netlify.com/sites/firefox-performance-dashboard/deploys/675a452e8b73a900085ac03b
😎 Deploy Preview https://deploy-preview-532--firefox-performance-dashboard.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@Carla-Moz Carla-Moz mentioned this pull request Dec 4, 2024
@92kns
Copy link
Member

92kns commented Dec 4, 2024

thanks @Carla-Moz ! just some quick questions before i take a deeper look
curious what were the reason(s) for modifying the yarn.lock file (was it related to the security stuff? if so I think that is outside the scope of this PR and should be done separately)

also curious how were the har files created/updated? that never happened for me (unless it was related to the yarn.lock changes, then nvm)

@Carla-Moz
Copy link
Author

thanks @Carla-Moz ! just some quick questions before i take a deeper look curious what were the reason(s) for modifying the yarn.lock file (was it related to the security stuff? if so I think that is outside the scope of this PR and should be done separately)

also curious how were the har files created/updated? that never happened for me (unless it was related to the yarn.lock changes, then nvm)

Actually, I didn't mean to include yarn.lock in the commit. Let me revert and see if that fixes things.

@Carla-Moz
Copy link
Author

@92kns And yes, I plan to fix the security concerns in a separate PR.

Copy link

codecov bot commented Dec 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.00%. Comparing base (630500e) to head (5b29ef8).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #532      +/-   ##
==========================================
+ Coverage   85.42%   86.00%   +0.57%     
==========================================
  Files          14       14              
  Lines         350      350              
  Branches       52       51       -1     
==========================================
+ Hits          299      301       +2     
+ Misses         46       44       -2     
  Partials        5        5              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -23,7 +23,7 @@ jobs:
command: yarn build
name: Check build
- run:
command: yarn test:coverage
command: yarn test:coverage -u
Copy link
Member

Choose a reason for hiding this comment

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

I am not too familiar with this - what is/was the reason to do this?

Copy link
Author

Choose a reason for hiding this comment

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

Like I said in our previous, I was force updating the snapshots but I'll remove this because this should only be done locally.

@92kns
Copy link
Member

92kns commented Dec 10, 2024

also, could you please squash the commits? might be easiest to squash to 1-3 commits (up to you) and force push

@92kns
Copy link
Member

92kns commented Dec 10, 2024

@Carla-Moz Revisiting the old ticket looks like we had this requirement I forgot to bring up again from https://bugzilla.mozilla.org/show_bug.cgi?id=1910645#c1

"We should also add a link to an FAQ (perhaps in the GitHub README) from the top of the page, and include a brief explainer of what we're showing. Something like "Using the most recent build available to us of the relevant browsers" and also touch on how/why we're building Chromium as release."

could you look into adding something like that?

@Carla-Moz
Copy link
Author

Carla-Moz commented Dec 12, 2024

@Carla-Moz Revisiting the old ticket looks like we had this requirement I forgot to bring up again from https://bugzilla.mozilla.org/show_bug.cgi?id=1910645#c1

"We should also add a link to an FAQ (perhaps in the GitHub README) from the top of the page, and include a brief explainer of what we're showing. Something like "Using the most recent build available to us of the relevant browsers" and also touch on how/why we're building Chromium as release."

could you look into adding something like that?

Sure! First, do we already have the link to the FAQ? Second, I would need the answer to this question: how and why are we building Chromium as release?

@Carla-Moz Carla-Moz closed this Dec 12, 2024
@Carla-Moz Carla-Moz force-pushed the fxp-3678-update-browsers-on-awfy branch from 7d68928 to 3712e31 Compare December 12, 2024 01:25
@Carla-Moz Carla-Moz reopened this Dec 12, 2024
temporary fix for tests, will need new PR to handle test case
revert test changes in perfherder.test.js
changed perfherder.test.js
updated cyan color
Update config.yml

Review yarn.lock to previous version

temporary fix for tests, will need new PR to handle test cases
@Carla-Moz Carla-Moz force-pushed the fxp-3678-update-browsers-on-awfy branch from 7d68928 to 90acbf7 Compare December 12, 2024 01:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants