Closed Bug 1247324 Opened 4 years ago Closed 4 years ago

Don't hit the network for switchboard configs in automation

Categories

(Firefox for Android :: Testing, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 47
Tracking Status
firefox46 --- wontfix
firefox47 --- fixed
fennec 46+ ---

People

(Reporter: Margaret, Assigned: Margaret)

References

Details

Attachments

(1 file)

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?
Flags: needinfo?(nalexander)
Blocks: 1234693
(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?
Flags: needinfo?(nalexander) → needinfo?(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.

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?
Flags: needinfo?(liuche)
(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.
(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.)
Flags: needinfo?(gbrown)
Blocks: 1248083
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?
tracking-fennec: --- → ?
Flags: needinfo?(nalexander)
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.
Assignee: nobody → margaret.leibovic
Flags: needinfo?(nalexander)
Flags: needinfo?(liuche)
Flags: needinfo?(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?
Flags: needinfo?(gbrown) → needinfo?(jmaher)
Oh, there's also the in-progress work for a mock push server: bug 1244816.
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.
Flags: needinfo?(jmaher)
(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.
(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?
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.
Flags: needinfo?(nalexander)
Flags: needinfo?(gbrown)
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?
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).
Flags: needinfo?(gbrown)
(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.
(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...
Flags: needinfo?(gbrown)
Margaret, you probably need to set the env var in GeckoThread
Flags: needinfo?(gbrown)
Oops, I misunderstood -- probably need to read it out of the intent directly
tracking-fennec: ? → 46+
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.
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.
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.
Flags: needinfo?(nalexander)
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
Attachment #8720908 - Flags: review?(mark.finkle) → review+
Blocks: 1249384
Comment on attachment 8720908 [details]
MozReview Request: Bug 1247324 - Disable Switchboard in automation. r=gbrown,mfinkle

https://reviewboard.mozilla.org/r/35507/#review32167
Attachment #8720908 - Flags: review?(gbrown) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/6008843fb7ba
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Depends on: 1249827
Don't need to uplift this.
You need to log in before you can comment on or make changes to this bug.