-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
core(emulation): retire moto g4, use moto g power #14674
Conversation
We're also going to need to update DT settings when this is rolled since DT sets up emulation on it's own |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we update the user agent as well?
c20b85b
to
e71fb91
Compare
Lighthouse plans to use the Moto G Power as the default emulated device for version 10.0. This will require the Moto G Power to be in the emulated device list when running Lighthouse in DevTools. Lighthouse PR: GoogleChrome/lighthouse#14674 Bug: None Change-Id: I2335bf8bc933b84fe13f85ea585ffb7e5c7ee506 Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/4214256 Commit-Queue: Adam Raine <[email protected]> Reviewed-by: Connor Clark <[email protected]> Reviewed-by: Mathias Bynens <[email protected]>
This should accompany the release of Lighthouse 10.0 which switches the default mobile device to the Moto G power. Lighthouse PR: GoogleChrome/lighthouse#14674 Bug: None Change-Id: I84154e10181d9c02e32daa70893b7ff4ce5905aa Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/4219345 Reviewed-by: Connor Clark <[email protected]> Commit-Queue: Connor Clark <[email protected]>
}, | ||
length: 1, | ||
length: 2, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the new one, should we add an expectation for it now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's unrelated, just so happened that the other audit being tested here got caught up. Don't wanna add it here...cuz it is named specific to its audit... :/
@@ -163,12 +163,12 @@ | |||
"throttlingMethod": "simulate", | |||
"screenEmulation": { | |||
"mobile": true, | |||
"width": 360, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably good for this PR, but we should do another complete artifact refresh to capture any changes the screen adjustment had.
@@ -160,7 +160,7 @@ describe('PerfCategoryRenderer', () => { | |||
const oppElements = [...categoryDOM.querySelectorAll('.lh-audit--load-opportunity')]; | |||
expect(oppElements.map(e => e.id).sort()).toEqual(oppAudits.map(a => a.id).sort()); | |||
expect(oppElements.length).toBeGreaterThan(0); | |||
expect(oppElements.length).toMatchInlineSnapshot('7'); | |||
expect(oppElements.length).toMatchInlineSnapshot('6'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What was the change here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sure it was just image aspect ratio or responsive images no longer failing according to the artifacts.
ref #14256