Closed Bug 1029818 Opened 10 years ago Closed 10 years ago

[Search experiment] QA experiment add-on and sign-off for deployment

Categories

(Firefox :: Search, defect)

Product:
Firefox
Component:
Search
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Iteration:
33.2
Tracking Flags:
Tracking Status
firefox31 --- verified

People

(Reporter: Felipe, Assigned: kjozwiak)

References

Details

Attachments

(2 files)

First xpi for testing
44.54 KB, application/x-xpinstall
Details
Patch
15.35 KB, patch
MattN
: review+
Details | Diff | Splinter Review 
After we have the .xpi produced we should test and ensure that it works as intended, specially after it has been deployed to the staging server in bug 1029810. Deployment to production should be dependent on a sign-off from this testing.
Flags: firefox-backlog+
Hey Kamil, bsmedberg/mschifer suggested you're the best person to help out with testing here - bug 1029746's dependencies has some extra context, but ping Felipe/bsmedberg/I for more detail.
Sounds good, I'll coordinate with felipe and bsmedberg and start testing as soon as the XPI becomes available.
Attached file First xpi for testing — 
This is the first version of a final xpi that can be used to start testing. With it we should be able to test the following aspects of the system:

- On installation, the search provider should switch to Bing*
- If provider was switched, typing in the searchbar will make the autocomplete popup appear with the new footer explaining about this experiment
- This footer should appear every time the popup appears _during the first session_
- Unless the X button or the Learn More links were clicked
- If in the first session (i.e., after the first time firefox is closed), it was displayed an amount smaller than 5 times, it will continue showing up on newer sessions, until it reaches 5 times
   (search for "timesShown" in about:config to make it easier to play with this setting)
- Make sure the footer appears in all windows, including new ones that were created after the add-on was installed
- Using a profile where you know you have some form autocomplete data, test that the footer doesn't appear on web forms, like this: https://bugzilla.mozilla.org/attachment.cgi?id=8447079
- Check that the style of the footer looks good and is not broken on all OSes and different window themes


* there are two conditions in which the provider shouldn't switch. They are
 - if Google wasn't set as the current provider
 - if the Searchbar has been removed from the toolbar

- On uninstallation, the provider should be switched back to Google
  - as long as the user didn't choose a different provider


With this pure .xpi it's not possible to test:
  - that the experiment will terminate itself before uninstalling and display the footer again after restoring the provider
  - the randomness of choosing the different test/control groups

This will be possible when we test the add-on in Experiments mode and not in pure .xpi mode
Flags: needinfo?(kamiljoz)
Also, the footer should cease to appear if the user switches providers
I primarily reviewed main.js.

I have one potential issue, but this may be a UX/experiment decision that was decided elsewhere. We appear to be excluding people from the experiment who have customized away their search bar. I am surprised by this, because those people may still be searching using about:home or directly from the location bar.

I can imagine several reasons for this:
* We wanted to make sure that there was a place to show the notification anchor for this experiment
* We really wouldn't make a change for people who had customized away their search box
* It was just too much eng/QA complexity to deal with this case, so we cut it for expediency

I expect that this is a small minority of users and it won't affect the experiment results, but unless somebody explicitly decided that we didn't want to measure this case, it seems better to keep this case part of the experiment.
Flags: needinfo?(felipc)
One other thing that might make it easier to QA the test is to allow people to "force" a branch setting from the environment:

e.g. here: https://hg.mozilla.org/users/felipc_gmail.com/search-experiment/file/6d67ec594c5b/lib/main.js#l74 add:

let envbranch = Cc["@mozilla.org/process/environment;1"].getService(Ci.nsIEnvironment).get("MOZ_SEARCH_EXPERIMENT_BRANCH");
if (envbranch != "") {
  branch = parseInt(envbranch);
} else if (...)

If QA doesn't care though, then don't feel obligated to add this.
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #6)
> I primarily reviewed main.js.
> 
> I have one potential issue, but this may be a UX/experiment decision that
> was decided elsewhere. We appear to be excluding people from the experiment
> who have customized away their search bar. I am surprised by this, because
> those people may still be searching using about:home or directly from the
> location bar.
> 
> I can imagine several reasons for this:
> * We wanted to make sure that there was a place to show the notification
> anchor for this experiment
> * We really wouldn't make a change for people who had customized away their
> search box
> * It was just too much eng/QA complexity to deal with this case, so we cut
> it for expediency
> 
> I expect that this is a small minority of users and it won't affect the
> experiment results, but unless somebody explicitly decided that we didn't
> want to measure this case, it seems better to keep this case part of the
> experiment.

I thought I had read this as part of the requirements in one of the breakdown bugs, but I can't find it. The source appears to be myself in https://bugzilla.mozilla.org/show_bug.cgi?id=1029776#c0
Perhaps this was something stated on IRC? I'm not sure. Philipp, can you tell if not changing the provider for users who have customized away the searchbar was an actual decision?
Flags: needinfo?(felipc) → needinfo?(philipp)
Yeah, I put that there, and it was for Benjamin's first reason (our only notification is anchored there, people without it wouldn't see it).
(In reply to :Felipe Gomes from comment #8)
> (In reply to Benjamin Smedberg  [:bsmedberg] from comment #6)
> > I can imagine several reasons for this:
> > * We wanted to make sure that there was a place to show the notification
> > anchor for this experiment
> > * We really wouldn't make a change for people who had customized away their
> > search box
> > * It was just too much eng/QA complexity to deal with this case, so we cut
> > it for expediency

It was actually a combination of all of those :)
I agree though that we should still include those users (I'm assuming we have a way to filter them out later if they prove to be crass outliers).
Flags: needinfo?(philipp)
Including these users while also being able to easily tell them apart would be a non-trivial addition. What I propose is: if a user without a searchbar is picked for a cell which would display the notification, move this user to the cell without a notification. It will have the same end result (i.e., the provider was changed and no notification was displayed), and it won't pollute the data for users on the cells who got the notification.

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #7)
> One other thing that might make it easier to QA the test is to allow people
> to "force" a branch setting from the environment:

good idea, it was simple so I added that
We shouldn't group those users into the same group with the no-notification group.

What we can do is put them in a separate group "I would have shown a notification but I couldn't", so:

"0" - disqualified from experiment
"1" - control
"2" - bing nonotify
"3" - bing notify
"3a" - bing notify but can't
"4" - yahoo nonotify
"5" - yahoo notify
"5a" - yahoo notify but can't

That was we still have equal distribution of control groups, which is important for measurement validity, but we can still tell apart the notify and can't-notify cases.

If this is too complicated for our timeframe, I think we should bail for now (use the existing behavior).
Ok, done. I've set up the experiments server now in order to test this through the proper deployment channel.

Manifest: https://people.mozilla.org/~fgomes/searchtest/no-filter/firefox-manifest.json

This doesn't include the filter.js function (meant to filter users who haven't been active for the past month), because it would make it harder to test it with clean profiles. I'll set up a separate instance that contains the filter to specifically test it.
We will not be using a jsfilter for this experiment. I can publish a branch of the telemetry-experiment-server repo with the final start/end parameters.
Are we not? There was a requirement to only include users who have been active once a week in the past 4 weeks: bug 1029263 comment 1.  I developed this filter for that: https://hg.mozilla.org/users/felipc_gmail.com/search-experiment/file/tip/lib/filter.js
Going through the experiment right now using the manifest from comment #14. Will add results once completed.
Flags: needinfo?(kamiljoz)
No, we aren't. The exclusion of those users will happen entirely in the data-analysis pipeline. That's because the patches to make jsfilter work were not uplifted to 31.
Ah, alright
QA Whiteboard: [qa+]
Felipe, I'm just going to post the issues that I've found so far rather than waiting till I'm done going through it:

Issues:

* "Learn More" under the footer doesn't appears to be working at all, this should show the user more information and dismiss the footer at the same time
* "This version of Firefox Beta uses..." always appears even when it's on nightly
* the footer seemed to stop working now. I used to get them but now I don't receive it even after landing in [3]bing notify or [5]yahoo notify

I'm using the link from comment # 14 and the only preference I change is: experiments.manifest.cert.checkAttributes => false

I double checked the raw data under about:healthreport and made sure I landed into a notify group which didn't display the footer in the toolbar
Attached patch Patch — — Splinter Review 
Adding the code in patch form for review
Build Testing:

* experiment was installed on fx 31.0b5 [20140626181429] without any issues
* experiment was rejected on fx 32.0a2 [20140630004007] without any issues (aurora is not listed as a valid channel in the manifest)
** 1404153306834	Browser.Experiments.Experiments	DEBUG	ExperimentEntry #0::isApplicable() - id=fx-searchtest-en-beta31@mozilla.org - test 'channel' failed
* experiment was installed on fx 33.0a1 [20140630030202] without any issues

Search Code Testing:

Yahoo Search Engine:

* about:newtab => https://search.yahoo.com/search?p=Mozilla&ei=UTF-8&fr=moz35
* about:home => https://search.yahoo.com/search?p=Mozilla&ei=UTF-8&fr=moz35
* url bar => https://search.yahoo.com/search?p=Mozilla&ei=UTF-8&fr=moz35
* toolbar => https://search.yahoo.com/search?p=Mozilla&ei=UTF-8&fr=moz35

Bing Search Engine:

* about:newtab => http://www.bing.com/search?q=Mozilla&pc=MOZI&form=MOZTSB
* about:home => http://www.bing.com/search?q=Mozilla&pc=MOZI&form=MOZSPG
* url bar => http://www.bing.com/search?q=Mozilla&pc=MOZI&form=MOZLBR
* toolbar => http://www.bing.com/search?q=Mozilla&pc=MOZI&form=MOZSBR

General Test Cases:

* ensure that the experiment is appearing under both about:config and about:support
* ensure that "experiments.activeExperiment;true" once the experiment installed
* ensure that the experiment is not being added into "extensions.bootstrappedAddons"
* ensure that the experiment is still installed after closing/re-opening fx several times
* ensure that the experiment is still installed after restarting the machine
* ensure that all search engines fields are using the engine that was set by the experiment (toolbar, about:newtab, about:home, URL)
* ensure that all the different types of windows are using the search engine that the experiment has installed (new tabs, new windows, private windows and e10s)
* ensure that the user can tell which search engine is being used under about:home, about:newtab and the toolbar
* ensure that when an experiment is removed/uninstalled/expired, it appears as "disabled" under about:support
* ensure that when an experiment is removed/uninstalled/expired, it appears greyed out under about:addons
* ensure that once the experiment is removed/uninstalled/expired, the default search engine is restored to Google (if the user hasn't selected a different search engine in the meantime)
* ensure experiments.activeExperiment;false once the experiment has been removed/uninstalled/expired
* ensure that if you're in the notify group (checking raw data in healthreport), the footer is displayed in the toolbar
* ensure that if you're in the nonotify group (checking raw data in healthreport), there's no footer visible when using the toolbar
* ensure that if you're in the control group (checking raw data in healthreport), you're search engine isn't changed
* ensured the footer always appears during the first session unless the "x" is selected
* ensured the footer appears at least 5 different times (it should span multiple sessions if five isn't reached in the first session)

Used the following builds:
- http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/latest-beta/win32/en-US/
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014-06-30-03-02-02-mozilla-central/

Felipe, so these are the issues that I've found so far:

* [Issue # 1] the first two issues from comment # 20 were happening when the footer was appearing in the toolbar and are still valid when the footer appears once in a while

* [Issue # 2] I've landed in group 3 & 5 many times (notify group) I rarely get the footer anymore. Once in a while if I land in the notify group I'll see the footer appear in the tool bar but most of the time it never shows up. I've reproduced this with both Windows 8.1 and Windows 7 x64 on VM machines.

* [Issue # 3] removing the experiment by making it expire (move the date on the computer ahead about a month and a few days) will correctly remove the add-on but it will not restore the search engine back to Google. However removing the experiment via the "Remove" button does restore the search engine back to Google without any issues
** experiments.activeExperiment => false
** "Search Experiment" under about:addons is grayed out and appears as "Complete"
** Active => false under about:support
Comment on attachment 8448394 [details] [diff] [review]
Patch

Review of attachment 8448394 [details] [diff] [review]:
-----------------------------------------------------------------

Disclaimer: I can't say I'm that familiar with the add-on SDK so there is possibly subtle weirdness that I'm missing. I would have preferred a vanilla bootstrap extension like we do for hotfixes.

The coding style is different than normal but I guess that's fine.

For temporary code given the rush rs=me

::: lib/dropdownnote.js
@@ +83,5 @@
> +  let block = document.createElement('vbox');
> +  block.id = NOTIFICATION_ID;
> +  block.collapsed = true;
> +  let hboxtitle = document.createElement('hbox');
> +  //hboxtitle.setAttribute('flex', '1');

Nit: dead code

@@ +88,5 @@
> +  hboxtitle.setAttribute('align', 'center');
> +  hboxtitle.setAttribute('pack', 'center');
> +  let engineImage = document.createElement('image');
> +  engineImage.setAttribute('style', 'width: 16px; height:16px; margin-left: 6px');
> +  let iconURI = engine.getIconURLBySize(16,16);

Nit: space after comma

@@ +120,5 @@
> +  let labelString = prefs.experimentFinished
> +                    ? 'Your default search engine has been restored back to Google. We had temporarily switched it to ' + friendlyName + ' as part of a Firefox Beta Study.'
> +                    : 'This version of Firefox Beta uses ' + friendlyName + ' as the default search engine. You can change this setting at any time.';
> +
> +  let text  = document.createTextNode(labelString);

I think you can just set the textContent property to reduce a line but this should work too.

@@ +137,5 @@
> +  footer.appendChild(learnMore);
> +  block.appendChild(hboxtitle);
> +  block.appendChild(description);
> +  block.appendChild(footer);
> +  popup.appendChild(block);  

Nit: trailing whitespace
Attachment #8448394 - Flags: review+
I've got a pretty good guess of what was happening for the footer to not appear, and I pushed an updated version that should hopefully fix that. The other two issues (Learn More link and reset on expiration) should also be fixed.
* ensure that the experiment is installed but doesn't change the default providers when either Yahoo, Bing, Amazon, Ebay, Twitter, Wikipedia or DuckDuckGo are set as default
* ensure that the search provider is not changed when the experiment is uninstalled and someone was using either Yaoo, Bing, Amazon, Ebay, Twitter, Wikipedia as their default
* ensure that the search provider isn't reverted back to Google if the user changed their default provider during the experiment duration
* ensured selecting "Learn More" under the footer directed the user to the "Searh Bar" website in a new tab
** Checked against: Win 8 x64, Win 7 Pro x64, Win Vista Ultimate x64, Win XP Pro x64, Ubuntu 13.10, OSX 10.9.3
* ensure that there's no issues with the footers UI/UX
** Checked against: Win 8 x64, Win 7 Pro x64, Win Vista Ultimate x64, Win XP Pro x64, Ubuntu 13.10, OSX 10.9.3
* ensure that selecting "Learn more" in the footer won't display the footer next time, even if it hasn't been displayed five different times
* ensure that selecting "x" in the footer won't display the footer next time, even if it hasn't been displayed five different times
* ensure that the search provider isn't changed when the searchbar has been removed

It appears like the expiration issue is still happening. I've ran into it every time while using all the different platforms. Once the experiment expires, the search provider isn't being reverted back to Google. I can reproduce the issue using the following STR:
- install the experiment
- once another search provider has been chosen, close fx
- move the computers date ahead about 30+ days and re-open fx
- experiment is disabled under about:addons & about:support but the search provider is still being used rather than reverting back to Google

It also looks like the search providers are being changed even after the searchbar is removed and moved into the "Additional Tools and Features" section of the Customization Menu:
- install fx
- select the fx menu -> customize and move the searchbar into the "Additional Tools and Features" section
- install the experiment and you'll notice that the search provider has changed in about:home, about:newtab and the URL bar
Search providers are now intentionally being changed even if the search bar is customized away, see comment 6-14.

I'm debugging the expiration issue.
ok, I can verify that the onUnload hook is not called in the following scenario:

start the experiment
shut down Firefox
move the system clock beyond experiment end
start Firefox

The uninstall hook *is* called if you manually uninstall from the addon manager, and I think also if the clock overflows while Firefox is running.

Felipe points out that this is caused by unimplemented features in the addon SDK: http://hg.mozilla.org/mozilla-central/annotate/ffb8b976548b/addon-sdk/source/app-extension/bootstrap.js#l74
I manually patched the generated bootstrap.js to fix the expiration issue and verified that it works. cset: https://hg.mozilla.org/users/felipc_gmail.com/search-experiment/rev/076854749d7b

I've pushed this last version to my "staging" at https://people.mozilla.org/~fgomes/searchtest/no-filter/firefox-manifest.json
I went through the search bar portion again and made sure that the search provider is changed even though the search bar has been removed. I made sure that the users are landing in the correct branches (1, 2, 3a, 4 and 5a). This is working correctly as mentioned in comment #26.

I went through the expiration portion and the search provider is now being reverted once the experiment has expired. I ensured that the search provider was reverted under about:home, about:newtab, search bar and the URL bar.

However, it appears that the experiment is expiring nine days prematurely. Once the experiment reaches 9 days, it expires and reverts the search provider.

STR the issue: (100% reproducible)
* install the search experiment using the link from comment #28
* once installed, move the computers date ahead by 17 days (11 days remaining)
* move the computers date ahead by 1 day (10 days remaining)
* move the computers date ahead by 1 day (there should be 9 days remaining but the experiment expires)
1405882887653	Browser.Experiments.Experiments	DEBUG	ExperimentEntry #0::isApplicable() - id=fx-searchtest-en-beta31@mozilla.org - test 'maxStartTime' failed

So the maxStartTime test is being incorrectly applied to a currently-running experiment.

I think that we should remove maxStartTime for now, deploy the experiment, and I'll file a followup so that we can fix maxStartTime.
Filed bug 1032970 on the maxStartTime issue.
The manifest without maxStartTime has been pushed to the staging server, and i've just pushed it to my manifest too.
Assignee: nobody → kamiljoz
Added to Iteration 33.2 for tracking of QA efforts.
Status: NEW → ASSIGNED
Iteration: --- → 33.2
Used the following staging server for verification:
- https://telemetry-experiment-dev.allizom.org/manifest/v1/firefox/%VERSION%/%CHANNEL%

* went through all the test cases outlined in comment #22, comment #25, comment #29
* ensured that the experiments behavior matches the correct branch (used about:healthreport to find what branch was installed)
* ensured that when there's three days remaining in the experiment, Google is changed back to the default search provider under about:home, about:newtab, url toolbar, search bar
* ensured that when there's three days remaining in the experiment, Google is not changed if a user is already using a different search provider other than Google
* ensured that when there's three days remaining in the experiment, users receive a footer under the search bar indicating that the search provider has been restored
* ensured that the footer appears 5 different times and spans multiple sessions
* ensured that selecting "x" removes the footer and doesn't appear under the search bar anymore
* ensured that when there's three days remaining in the experiment, users that are using another search provider other than Google don't receive the footer
* ensured that when there's three days remaining in the experiment, users that are in branch 1 don't receive the footer
* ensured that selecting "learn more" under the footer opens a new tab and takes the user to the correct website (got an error message but this was because my computers date was incorrect)

Possible issues:

* When you move the computers date so there's three days remaining, you'll always receive the footer in the search bar. If you move the computers date so there's 2 or 1 day(s) remaining, you won't receive the footer:

STR:
* move the machines date so there's three days remaining in the experiment -> reverts to Google and displays the footer
* move the machines date so there's two days remaining in the experiment -> reverts to Google but doesn't display the footer
* move the machines date so there's one day remaining in the experiment -> reverts to Google but doesn't display the footer
After speaking with felipe on IRC, the above isn't an issue. If you're placed in either branch #1, #2 or #4, you won't receive the footer. However, if you're placed inside branch #3 or #5, you'll receive the footer. I went through this portion of testing again and confirmed that that's the case.

Branches #1, #2, #4 => no footer when Google is restored when experiment has three days remaining [Passed]
Branches #3, #5 => footer appears when Google is restored when experiment has three days remaining [Passed]

Looks like that's it! All the other issues have been addressed/fixed and everything looks like it's working correctly.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Tested via comment #20, comment #22, comment #25, comment #29, comment #34 and comment #35.
Status: RESOLVED → VERIFIED
QA Whiteboard: [qa+] → [qa!]
status-firefox31: --- → verified
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: