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

Allow Firefox & Chrome to use fake media stream #13062

Merged
merged 2 commits into from Sep 19, 2018
Merged

Conversation

Hexcles
Copy link
Member

@Hexcles Hexcles commented Sep 18, 2018

This is yet another difference in results I observed between BuildBot and Taskcluster.

This was added to wpt.fyi in web-platform-tests/results-collection#504 .

Copy link
Member

@foolip foolip left a comment

Choose a reason for hiding this comment

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

Thanks! Perhaps @jugglinmike would also like to review?

@@ -221,6 +221,9 @@ def setup_kwargs(self, kwargs):
channel=kwargs["browser_channel"])
kwargs["prefs_root"] = prefs_root

# Allow WebRTC tests to call getUserMedia.
kwargs["extra_prefs"].append("media.navigator.streams.fake=true")
Copy link
Contributor

Choose a reason for hiding this comment

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

Honestly I think this should be in https://searchfox.org/mozilla-central/source/testing/profiles/unittest/user.js or the wpt-specific equivalent that @whimboo is adding. But I guess here is OK temporarily if it's inconvenient to make an upstream patch.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd be happy to make a patch once @whimboo creates the wpt-specific profile, and I'll remove this afterwards. In the interim, let's land this to make Taskcluster results more useful.

Copy link
Contributor

Choose a reason for hiding this comment

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

My patch has already been landed, and was merged upstream via #13050.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Hexcles Hexcles merged commit 7e858cd into master Sep 19, 2018
@Hexcles
Copy link
Member Author

Hexcles commented Sep 19, 2018

Darn, meant to squash-merge but clicked the wrong button. Sorry.

@Hexcles Hexcles deleted the get-user-media branch September 19, 2018 19:36
@jugglinmike
Copy link
Contributor

I'm a little late, but I'm still interested in hearing your perspective.

This makes the browser behave less realistically, so I'm concerned about making it the default behavior for users of the WPT CLI. It might be confusing for folks to understand discrepancies in results reported by the CLI and by running manually or with a different automation tool. And if they wanted to learn more, they'd have to research the source code (rather than read a log file).

What do you think about maintaining this at the edges? Specifically, that would mean adding the browser-specific flags to the TaskCluster script.

@Hexcles
Copy link
Member Author

Hexcles commented Sep 19, 2018

Sorry for the rush, @jugglinmike .

What do you think about maintaining this at the edges? Specifically, that would mean adding the browser-specific flags to the TaskCluster script.

IMHO, this merely shuffles the confusion around, from discrepancy between manual runs (or other runners) and wpt run, to discrepancy between wpt.fyi and wpt run.

Our current setup until very recently (i.e. on BuildBot) is more like the latter, but I'd argue that the former is slightly better, for the following reasons:

  • wpt run is the canonical way to run tests. Running tests "manually" (without automation) isn't trivial, as one needs to at least change settings like popup blockers, certificates, etc. And it's the other runners' responsibilities to replicate upstream's behaviours as they made the tradeoff not to use the upstream runner (as a maintainer of the Blink test runner, I can say this).
  • Firefox already has some special prefs for testing, e.g. with a few experimental features enabled. In other words, browsers started by wpt run have always been behaving somewhat differently from the defaults.
  • However, the difference between wpt run and wpt.fyi is more surprising. One would expect a dashboard for WPT should be an honest representation of results from the wpt runner. Yet we have some magic flags specific to wpt.fyi that aren't documented and would require digging into another repo to find out.
  • The lower level an engineer goes, the more they'd tend to know. And I'd say running the test manually is one level lower than using the wpt frontend, and is most likely for debugging. For someone debugging WebRTC tests, they have a better chance to know fake devices aren't enabled by default (and how to enable it).

Feel free to respond to my reasoning above. This is certainly something worth discussing!

@jgraham
Copy link
Contributor

jgraham commented Sep 20, 2018

I fully agree with what @Hexcles says.

I agree that there's a concern about making the test configuration too different from the real browser, and I would like to reduce the number of differences we have for Firefox by cutting down the profile settings we apply (removing ones that just enable features that aren't on by default in nightly for example). However, enabling fake input streams so that media tests can work seems pretty clearly on the side of acceptable test configuration.

@jugglinmike
Copy link
Contributor

Yet we have some magic flags specific to wpt.fyi that aren't documented and would require digging into another repo to find out.

We've been talking about providing a link to log files on wpt.fyi. That's the feature I've had in mind when comparing the discoverability of these two solutions. To me, logs are a more intentional way to communicate details about the runtime (compared with program source code, however well documented).

That's the third time we've mentioned documentation. I appreciate your perspective as a downstream consumer, and docs seems particularly relevant in that case. The concerns I have about result veracity would be at least partially mitigated if we were more forward about the things we do to support automation.

Before we can get there, I think a more general restructuring is in order--see gh-13125.

In the immediate term, we were previously recommending the use of these arguments for Chrome, so that should be removed: gh-13126

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.

None yet

6 participants