Skip to content

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

Merged
pmeenan merged 1 commit into
catchpoint:masterfrom
wildlyinaccurate:patch-2
May 29, 2020
Merged

Set --emulated-form-factor=desktop for LH tests without device emulation#297
pmeenan merged 1 commit into
catchpoint:masterfrom
wildlyinaccurate:patch-2

Conversation

@wildlyinaccurate

@wildlyinaccurate wildlyinaccurate commented Nov 5, 2019

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

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

what does self.options.android do?

@wildlyinaccurate

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

pmeenan commented Nov 8, 2019

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

pmeenan commented Dec 16, 2019

Copy link
Copy Markdown
Contributor

@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
Copy Markdown
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