1247324 - Don't hit the network for switchboard configs in automation

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Testing, defect)

Description

[:Margaret Leibovic]

While working on bug 1234693, I discovered that we're downloading the switchboard config over the network while running tests in automation.

We want to be able to test configurations that we're shipping to users, so in my opinion this is better than having no active switchboard experiments in automation, but this is still not good.

Nick, can you help us figure out what we should do here?

Comment 1

[Nick Alexander :nalexander [he/him]]

(In reply to :Margaret Leibovic from comment #0)
> While working on bug 1234693, I discovered that we're downloading the
> switchboard config over the network while running tests in automation.
> 
> We want to be able to test configurations that we're shipping to users, so
> in my opinion this is better than having no active switchboard experiments
> in automation, but this is still not good.
> 
> Nick, can you help us figure out what we should do here?

This is hard in general.  For the Push work, I'm explicitly managing the push configuration on the Gecko side and propagating it to the Android side.  That doesn't make sense for first-run stuff, including experiments.

I think the expedient thing to do is to define an environment variable specifying Switchboard configuration (perhaps enabled and optionally endpoint URL to use), and then set it in the test harnesses.  Like at https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/runrobocop.py#401, although we probably want this for *all* test harnesses (and not just Robocop).

We could also find a way for test harnesses to write shared preference files before test runs, and then use shared prefs.  Adding yet another way to configure things without clear use cases seems unnecessary.

It's worth thinking about what happens if you *change* Switchboard servers between browser runs.  Is that an issue?

gbrown: do you have recommendations for this kind of test-only configuration?

Comment 2

[:Margaret Leibovic]

(In reply to Nick Alexander :nalexander from comment #1)
> (In reply to :Margaret Leibovic from comment #0)
> > While working on bug 1234693, I discovered that we're downloading the
> > switchboard config over the network while running tests in automation.
> > 
> > We want to be able to test configurations that we're shipping to users, so
> > in my opinion this is better than having no active switchboard experiments
> > in automation, but this is still not good.
> > 
> > Nick, can you help us figure out what we should do here?
> 
> This is hard in general.  For the Push work, I'm explicitly managing the
> push configuration on the Gecko side and propagating it to the Android side.
> That doesn't make sense for first-run stuff, including experiments.

FYI, the first-run experiments don't come over the network. They're defined locally. And I believe we explicitly disable the first-run UI in automation. Chenxia, is this correct?

Comment 3

[:Margaret Leibovic]

(In reply to Nick Alexander :nalexander from comment #1)

> It's worth thinking about what happens if you *change* Switchboard servers
> between browser runs.  Is that an issue?

This shouldn't matter, as long as the servers are serving the correctly formatted config data. We expect configurations to change between browser runs (that's the point of having a remotely hosted configuration), so the client should be able to handle this.

Comment 4

[Geoff Brown [:gbrown]]

(In reply to Nick Alexander :nalexander from comment #1)
> (In reply to :Margaret Leibovic from comment #0)
> > While working on bug 1234693, I discovered that we're downloading the
> > switchboard config over the network while running tests in automation.
> > 
> > We want to be able to test configurations that we're shipping to users, so
> > in my opinion this is better than having no active switchboard experiments
> > in automation, but this is still not good.
> > 
> > Nick, can you help us figure out what we should do here?
> 
> This is hard in general.  For the Push work, I'm explicitly managing the
> push configuration on the Gecko side and propagating it to the Android side.
> That doesn't make sense for first-run stuff, including experiments.
> 
> I think the expedient thing to do is to define an environment variable
> specifying Switchboard configuration (perhaps enabled and optionally
> endpoint URL to use), and then set it in the test harnesses.  Like at
> https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/runrobocop.
> py#401, although we probably want this for *all* test harnesses (and not
> just Robocop).
> 
> We could also find a way for test harnesses to write shared preference files
> before test runs, and then use shared prefs.  Adding yet another way to
> configure things without clear use cases seems unnecessary.
> 
> It's worth thinking about what happens if you *change* Switchboard servers
> between browser runs.  Is that an issue?
> 
> gbrown: do you have recommendations for this kind of test-only configuration?

We would normally use test-only prefs, pushed to the profile, for manipulating test-only behavior. If that's not possible or highly inconvenient for the Switchboard, I think an environment variable is the way to go. (Prefs are preferred because they can be shared between test harnesses, they are the normal mechanism; environment vars may end up being passed on the adb command line, which is already long and has a hard maximum length.)

Comment 5

[:Margaret Leibovic]

We should really address this before we start doing more with switchboard. Ideally we should have a system to let us test every possible switchboard config in automation... or at the very least we should test different configurations of each feature for relevant tests (e.g. menu tests should test both on/off configurations of a menu experiment).

Chenxia or Nick, can one of you own this bug?

Comment 6

[:Margaret Leibovic]

My short-term proposal here is to make the switchboard URLs configurable at runtime, and mock out a testing switchboard server with mochitest.

gbrown, do you have advice on the best way to do this? Do we have examples of other remote servers that we mock out for test runs?

This isn't great because it will only test one configuration, but at least it will remove the dependency on the remote server.

After that, we could create different testing configs, and point the URLs to those different configs for different test runs related to those experiments.

My hope is that we won't be doing anything so crazy with switchboard that turning a feature on/off will bust the app. I also don't want us to have switchboard experiments that depend on each other, so we should be able to test individual experiments in their on/off states without needing to worry about a complex matrix of experiment interactions.

Comment 7

[Geoff Brown [:gbrown]]

> gbrown, do you have advice on the best way to do this? Do we have examples of other remote servers that we
> mock out for test runs?

The closest thing I can think of are the xpcshell-based webserver, ssl, and web socket servers used for mochitests:

http://hg.mozilla.org/mozilla-central/annotate/6ea654cad929/testing/mochitest/runtests.py#l822

> After that, we could create different testing configs, and point the URLs to those different configs for
> different test runs related to those experiments.

Different try and/or local test runs only, right? All tests on inbound, central, etc would all run against a consistent config?

Isn't the switchboard similar to desktop "experiments"? It looks like those are basically disabled in tests: http://hg.mozilla.org/mozilla-central/annotate/709f559b5406/testing/profiles/prefs_general.js#l62. 

:jmaher -- Any thoughts?

Comment 8

[Geoff Brown [:gbrown]]

Oh, there's also the in-progress work for a mock push server: bug 1244816.

Comment 9

[Joel Maher ( :jmaher ) (UTC -8)]

We don't have a specific mock server that we run anywhere.  In the land of httpd.js, there is .sjs files:
https://dxr.mozilla.org/mozilla-central/search?q=path%3A.sjs&redirect=false&case=false

these files can act as a server and you can craft the response you expect based on the query parameters sent in.  you can control everything down to the exact response (nothing, 404, 302, etc.).

I am not sure if that would help, ideally creating switchboard prefs for the server and making httpd.js + .sjs act as the server should solve this.

Comment 10

[:Margaret Leibovic]

(In reply to Geoff Brown [:gbrown] from comment #7)
> > gbrown, do you have advice on the best way to do this? Do we have examples of other remote servers that we
> > mock out for test runs?
> 
> The closest thing I can think of are the xpcshell-based webserver, ssl, and
> web socket servers used for mochitests:
> 
> http://hg.mozilla.org/mozilla-central/annotate/6ea654cad929/testing/
> mochitest/runtests.py#l822
> 
> > After that, we could create different testing configs, and point the URLs to those different configs for
> > different test runs related to those experiments.
> 
> Different try and/or local test runs only, right? All tests on inbound,
> central, etc would all run against a consistent config?

When I wrote that comment, I was thinking that we could switch the config dynamically within a single test, so that we could exercise different testcases against different relevant configs. 

However, thinking about this more, maybe we should instead override the `isInExperiment` logic for a given test to let us test the different configurations. Or even abstract the switchboard logic away from the application logic enough to let us test the different application configurations without messing with switchboard.

> Isn't the switchboard similar to desktop "experiments"? It looks like those
> are basically disabled in tests:
> http://hg.mozilla.org/mozilla-central/annotate/709f559b5406/testing/profiles/
> prefs_general.js#l62. 

Do you know how desktop experiments are tested then? Are they tested in their own infrastructure?

I think we should do something similar to this and basically serve up an empty switchboard config for the testing profile. Then we can figure out how to test the experiments themselves separately.

Comment 11

[Geoff Brown [:gbrown]]

(In reply to :Margaret Leibovic from comment #10)
> Do you know how desktop experiments are tested then? Are they tested in
> their own infrastructure?

I really don't know. Maybe just ad-hoc local tests?

Comment 12

[:Margaret Leibovic]

After chatting with nalexander, I think the approach we should take here is an environment variable to just disable switchboard in testing profiles.

This would address the most immediate concern of preventing network requests from happening in automation. We can file a follow-up bug to make sure we're properly testing switchboard experiments themselves, but maybe that should happen independently of switchboard (i.e. test variations in application logic directly, instead of by changing the switchboard config that controls those variations).

We already have a build flag for switchboard, so maybe we could read the environment variable to set the value in AppConstants, if the build flag is true:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/AppConstants.java.in#328

However, I'm not wondering if this build flag even works properly, given that we use Switchboard.isInExperiment calls directly in the code without an AppConstants.MOZ_SWITCHBOARD check... but that would be a separate issue to resolve.

Comment 13

[:Margaret Leibovic]

And I forgot to ask my question... what should I do to set this environment variable in the test harness? Should I just set it in here?
https://dxr.mozilla.org/mozilla-central/source/build/mobile/remoteautomation.py#87

Or are there other things I would need to do?

Comment 14

[Geoff Brown [:gbrown]]

That's the place. I would name it MOZ_something, making <something> as short (fewer characters) as you reasonably can. I think that's all you'll need to get that env var defined for all browser tests (robocop, mochitest, and all reftests, but not xpcshell or cppunit...which I don't think you need to worry about).

Comment 15

[:Margaret Leibovic]

(In reply to :Margaret Leibovic from comment #12)

> However, I'm not wondering if this build flag even works properly, given
> that we use Switchboard.isInExperiment calls directly in the code without an
> AppConstants.MOZ_SWITCHBOARD check... but that would be a separate issue to
> resolve.

Looking at the comment near this variable declaration, we always build Switchboard, but this flag just determines whether or not we enable it.

So, I think it's fine to have Switchboard.isInExperiment calls that aren't wrapped in an AppConstants.MOZ_SWITCHBOARD check. isInExperiment should just return false in these cases, since Switchboard hasn't been initialized.

I don't want to need to sprinkle MOZ_SWITCHBOARD checks all over the place, so I think we should just continue with what we've been doing.

Comment 16

[:Margaret Leibovic]

(In reply to Geoff Brown [:gbrown] from comment #14)
> That's the place. I would name it MOZ_something, making <something> as short
> (fewer characters) as you reasonably can. I think that's all you'll need to
> get that env var defined for all browser tests (robocop, mochitest, and all
> reftests, but not xpcshell or cppunit...which I don't think you need to
> worry about).

Thanks, I was able to set this environment variable, and I see it being set in the log.

However, I'm finding that BrowserApp.onCreate is being called before this environment variable is set, so Switchboard is still being initialized... is there any way to make sure we set an environment variable before launching Fennec?

Otherwise, I'm not sure how to short-circuit the Switchboard requests...

Comment 17

[James Willcox (:snorp) ([e-mail]) (he/him)]

Margaret, you probably need to set the env var in GeckoThread

Comment 18

[James Willcox (:snorp) ([e-mail]) (he/him)]

Oops, I misunderstood -- probably need to read it out of the intent directly

Comment 19

[Geoff Brown [:gbrown]]

The harness should be starting Fennec with "--es env<N> <your-var>=<your-value>". I think that's the best we can do from the harness. :snorp's probably right about the intent, but I haven't tried that.

Comment 20

[:Margaret Leibovic]

I'll try getting it out of the intent. Switchboard is initialized in onCreate, so at least I have easy access to the intent there and can turn it off.

Comment 21

[:Margaret Leibovic]

Attachment: MozReview Request: Bug 1247324 - Disable Switchboard in automation. r=gbrown,mfinkle

Review commit: https://reviewboard.mozilla.org/r/35507/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/35507/

Comment 22

[:Margaret Leibovic]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8b299a4df5fe

Comment 23

[:Margaret Leibovic]

I verified locally with logging that this disables switchboard when running a robocop test in the emulator.

We could refactor the earlyStartJavaSampler logic from GeckoApp, since that's where I copied this from, but I wanted to keep this as simple as possible for uplift.

Comment 24

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 8720908 [details]
MozReview Request: Bug 1247324 - Disable Switchboard in automation. r=gbrown,mfinkle

https://reviewboard.mozilla.org/r/35507/#review32165

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:567
(Diff revision 1)
> +    private static boolean isSwitchboardDisabled(SafeIntent intent) {

I wonder if this would be nice to move to Experiments.java so we could cache that Switchboard is disabled and use Experiments.isDisabled() as needed in the code.

Just a thought

Comment 25

[Geoff Brown [:gbrown]]

Comment on attachment 8720908 [details]
MozReview Request: Bug 1247324 - Disable Switchboard in automation. r=gbrown,mfinkle

https://reviewboard.mozilla.org/r/35507/#review32167

Comment 26

[:Margaret Leibovic]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=01496109f033

Comment 27

[:Margaret Leibovic]

I see that Switchboard is disabled on try when I look at the logs:
02-18 13:10:54.111   537   537 D GeckoExperiments: Switchboard disabled by MOZ_DISABLE_SWITCHBOARD environment variable

This gives me confidence to land this.

Comment 28

[:Margaret Leibovic]

https://hg.mozilla.org/integration/fx-team/rev/6008843fb7ba5de8a2c7e39bf6e2e4a2e7f1532b
Bug 1247324 - Disable Switchboard in automation. r=gbrown,mfinkle

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6008843fb7ba

Comment 30

[:Margaret Leibovic]

Don't need to uplift this.