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

Set --emulated-form-factor=desktop for LH tests without device emulation #297

Merged
merged 1 commit into from
May 29, 2020
Merged

Set --emulated-form-factor=desktop for LH tests without device emulation #297

merged 1 commit into from
May 29, 2020

Conversation

wildlyinaccurate
Copy link
Contributor

@wildlyinaccurate wildlyinaccurate commented Nov 5, 2019

WPT currently runs desktop LH tests with --emulated-form-factor none which has a few side effects including dropping the Chrome-Lighthouse user-agent modifier. This makes it impossible to whitelist the LH run in bot managers.

This patch uses --emulated-form-factor desktop instead. This is consistent with PageSpeed Insights.

@connorjclark
Copy link
Contributor

Confirming that Lighthouse does not emulate the UA string when emulatedFormFactor is not set to mobile or desktop. This was unexpected to me, but @paulirish says it is the expected behavior.

Setting this option to desktop for a real mobile device will have unintended consequences (the viewport will be much bigger). I'm afraid currently, there is no way to disable device emulation but keep UA strings.

@wildlyinaccurate
Copy link
Contributor Author

Setting this option to desktop for a real mobile device will have unintended consequences (the viewport will be much bigger).

@connorjclark just to be clear: this changeset would only set it to desktop when the WPT mobile test flag is not truthy. Are there cases where WPT would run tests on a real mobile device without setting the mobile flag?

@connorjclark
Copy link
Contributor

hmm I'm not familiar enough with WPT to say.

what does self.options.android do?

@wildlyinaccurate
Copy link
Contributor Author

what does self.options.android do?

I'm glad you asked. I think the intention is to disable emulation on Android devices, so I'll need to update this patch to retain that functionality.

@patrickhulce
Copy link
Contributor

Hi there! I don't have much to add here other than to express that I'm another concerned party that would like to be extra, extra sure that this would not change anything for mobile emulated or real mobile runs at all before merging :)

@wildlyinaccurate
Copy link
Contributor Author

@patrickhulce the fact that two Googlers are expressing concern over this PR has made me slightly nervous and is making me question my grasp of boolean logic.

As far as I understand it, the current behaviour is:

  • Desktop tests: --emulated-form-factor=none
  • Emulated mobile tests: no flag present (implies --emulated-form-factor=mobile)
  • Real android device tests: --emulated-form-factor=none
  • Real iOS device tests: N/A since LH can't run

The only change this PR makes is:

  • Desktop tests: --emulated-form-factor=desktop

@patrickhulce
Copy link
Contributor

Haha, sorry @wildlyinaccurate didn't mean to alarm you! :) I haven't spent time in the WPT codebase in a while so my memory is foggy on what is set in which situations and the specifics of the integration here, but just wanted to flag it.

If the situation you've laid out is true, then it must mean that the emulated mobile case is already using the standard nexus 5x UA regardless of the emulated device in the main WPT run?

@pmeenan
Copy link
Contributor

pmeenan commented Nov 8, 2019

I'll have to look closed but I'd much rather just add the UA string explicitly (through the command-line or when the PTST UA string is added) than force either emulation mode. It feels like either emulation mode is likely to carry other baggage with it.

@wildlyinaccurate
Copy link
Contributor Author

@pmeenan that's the strange thing - we're already doing that for desktop LH runs but the PTST UA modifier is not present in the LH results.

The aim of this PR was to at least make sure the Chrome-Lighthouse modifier was present, since we seem unable to set the UA ourselves.

@pmeenan
Copy link
Contributor

pmeenan commented Dec 16, 2019

@wildlyinaccurate It's possible that the UA string (even on the command-line) is being overridden here: https://github.com/WPO-Foundation/wptagent/blob/master/internal/devtools_browser.py#L134

It might be worth testing to see if that code could detect it is running a lighthouse test and add the modifier to the UA string itself.

I believe the intention of setting --emulated-form-factor=none is to ask LH to run a "desktop" test, since the default value of this flag is "mobile". However, setting it to "none" seems to have some unintended side effects like preventing LH from adding the "Chrome-Lighthouse" user-agent modifier.

This would fix #296.
@wildlyinaccurate
Copy link
Contributor Author

I finally got around to testing this. Confirmed behaviour:

  • --emulated-form-factor none used for tests where android=1
  • --emulated-form-factor mobile used for tests where mobile=1 and android=0
  • --emulated-form-factor desktop used for tests where mobile=0 and android=0

Also confirmed that the Chrome-Lighthouse UA modifier is now present on desktop runs.

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.

4 participants