Closed Bug 1166132 Opened 9 years ago Closed 9 years ago

Create new talos test to measure tab switching times

Categories

(Testing :: Talos, defect)

defect
Not set
normal

Tracking

(e10s+, firefox42 fixed)

RESOLVED FIXED
mozilla42
Tracking Status
e10s + ---
firefox42 --- fixed

People

(Reporter: mconley, Assigned: jmaher)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 11 obsolete files)

18.35 KB, patch
mconley
: review+
Details | Diff | Splinter Review
2.26 KB, patch
mconley
: review+
Details | Diff | Splinter Review
In order to fix bug 1135719, it's important for us to set benchmarks and measure.

We currently have Telemetry probes out in the wild (bug 1156592) helping us get a sense of what kind of tab switch times our Telemetry-enabled users are experiencing. We'll hopefully be getting more accurate tab switching times for non-e10s in bug 1161758 to compare against.

That's great for making sure our changes are impacting real users, but we should also get a talos test written so that we can detect when things are improving or worsening tab switch times without having to wait for our users to submit telemetry data about it.
Blocks: 1040081
Depends on: 1173426
Attached patch tps.patch (obsolete) — Splinter Review
Assignee: nobody → blassey.bugs
Attachment #8621067 - Flags: review?(mconley)
Comment on attachment 8621067 [details] [diff] [review]
tps.patch

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

Stoked for this test. Just some minor issues. Nothing critical from me.

I think avih should also take a look so he knows what's up, and we make sure we're getting the right stuff / not missing something crucial.

::: talos/pageswitch/bootstrap.js
@@ +33,5 @@
> +}
> +
> +function executeSoon(callback) {
> +  Services.tm.mainThread.dispatch(callback, Components.interfaces.nsIThread.DISPATCH_NORMAL);
> +}

Nit - newline between functions

@@ +44,5 @@
> +    }
> +  }, topic, false);
> +}
> +
> +function waitForTabLoads(browser, numTabs, callback) {

Alternatively, use a Promise instead of a callback, but I don't care that much.

@@ +45,5 @@
> +  }, topic, false);
> +}
> +
> +function waitForTabLoads(browser, numTabs, callback) {
> +  var listener = {

let, not var

@@ +51,5 @@
> +    QueryInterface: XPCOMUtils.generateQI(["nsIWebProgressListener",
> +                                           "nsISupportsWeakReference"]),
> +
> +    onStateChange: function(aBrowser, aWebProgress, aRequest, aStateFlags, aStatus) {
> +       let loadedState = Ci.nsIWebProgressListener.STATE_STOP |

Nit - two space indentation for this block

@@ +54,5 @@
> +    onStateChange: function(aBrowser, aWebProgress, aRequest, aStateFlags, aStatus) {
> +       let loadedState = Ci.nsIWebProgressListener.STATE_STOP |
> +        Ci.nsIWebProgressListener.STATE_IS_NETWORK;
> +       if ((aStateFlags & loadedState) == loadedState &&
> +                !aWebProgress.isLoadingDocument &&

busted indentation

@@ +58,5 @@
> +                !aWebProgress.isLoadingDocument &&
> +          aWebProgress.DOMWindow == aWebProgress.DOMWindow.top) {
> +         this.count++;
> +         if (this.count == numTabs) {
> +           //browser.removeTabsProgressListener(listener);

dead code

@@ +71,5 @@
> +    onProgressChange: function(aWebProgress, aRequest, curSelf, maxSelf, curTot, maxTot) {},
> +    onStatusChange: function(aWebProgress, aRequest, aStatus, aMessage) {},
> +    onSecurityChange: function(aWebProgress, aRequest, aState) {}
> +  }
> +  //tab.linkedBrowser.addProgressListener(listener);

dead code

@@ +86,5 @@
> +}
> +
> +function runTest(tabs, win, callback) {
> +  let startTab = win.gBrowser.selectedTab;
> +  let start = Date.now();

Instead of using Date.now(), we should probably use the high-resolution window.performance.now() or equivalent.

@@ +88,5 @@
> +function runTest(tabs, win, callback) {
> +  let startTab = win.gBrowser.selectedTab;
> +  let start = Date.now();
> +  runTestHelper(startTab, tabs, 0, win, function() {
> +    callback(Date.now() - start);

Again, win.performance.now() or equivalent for the subtraction.

@@ +104,5 @@
> +  });
> +  win.gBrowser.selectedTab = tab;
> +}
> +
> +var gTestURLs;

Should probably default this to an empty Array

@@ +108,5 @@
> +var gTestURLs;
> +
> +function test(window) {
> +  let win = window.OpenBrowserWindow();
> +  if (!gTestURLs) {

This is repeated down below. If we really need to run this now, we should extract both to a common function.

@@ +121,5 @@
> +  }
> +  whenDelayedStartupFinished(win, function() {
> +    loadTabs(gTestURLs, win, function(tabs) {
> +      runTest(tabs, win, function(time) {
> +        win.alert("test! " + time);

We'll need to somehow hand this over to talos. I think talos wants us to dump this to the console for it to parse... avih would probably know better.

@@ +140,5 @@
> +function loadIntoWindow(window) {
> +  if (!window)
> +    return;
> +  let item = window.document.createElement("menuitem");
> +  item.setAttribute("label","Start test");

Nit - space after ,

@@ +161,5 @@
> +    return;
> +  }
> +
> +  let resource = Services.io.getProtocolHandler("resource").QueryInterface(Ci.nsIResProtocolHandler);
> +  resource.setSubstitution("myaddonpackage", null);

We can probably use something more descriptive - like tabswitcher or something.

@@ +175,5 @@
> +  Services.obs.removeObserver(observer, "tabswitch-urlfile");
> +}
> +
> +function handleFile(win, file) {
> +  var localFile = Components.classes["@mozilla.org/file/local;1"]

let instead of var throughout this function - and you can use Cc and Ci here instead.

@@ +183,5 @@
> +  var localURI = Services.io.newFileURI(localFile);
> +  req.open('get', localURI.spec, false);
> +  req.send(null);
> +  var parent = localURI.spec.split(localFile.leafName)[0];
> +  var lines = req.responseText.split('<a href = \"');

This seems kinda fragile...

Dunno if this is any better, but maybe try:

win.document.createElement("body");
body.innerHTML = req.responseText;
let anchors = body.querySelectorAll("a");
let links = [...anchors].map((anchor) => {
  return anchor.getAttribute("href");
});

links.forEach(function(link) {
  gTestURLs.push(parent + link);
});

@@ +184,5 @@
> +  req.open('get', localURI.spec, false);
> +  req.send(null);
> +  var parent = localURI.spec.split(localFile.leafName)[0];
> +  var lines = req.responseText.split('<a href = \"');
> +  gTestURLs = new Array();

gTestUrls = [];

@@ +186,5 @@
> +  var parent = localURI.spec.split(localFile.leafName)[0];
> +  var lines = req.responseText.split('<a href = \"');
> +  gTestURLs = new Array();
> +  lines.forEach(function(a) {
> +    //Components.utils.reportError(parent + a.split('\"')[0]);

Dead code

@@ +218,5 @@
> +
> +  // Load into any new windows
> +  Services.wm.addListener(windowListener);
> +  if (window) {
> +    var prefs = Components.classes["@mozilla.org/preferences-service;1"]

Services.prefs can be used instead.

@@ +220,5 @@
> +  Services.wm.addListener(windowListener);
> +  if (window) {
> +    var prefs = Components.classes["@mozilla.org/preferences-service;1"]
> +      .getService(Components.interfaces.nsIPrefBranch);
> +    var prefFile = prefs.getCharPref("addon.test.tabswitch.urlfile");

let rather than var - var hoisting scares the bejeebus out of me.

Also, this will throw if the pref is not there. You should probably try/catch, or check that the preference exists before attempting to read it.

::: talos/pageswitch/content/options.xul
@@ +1,3 @@
> +<?xml version="1.0" ?>
> +<vbox xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul">
> +  <setting pref="addon.test.tabswitch.urlfile" type="file" title="Stock to watch" oninputchanged="Components.classes['@mozilla.org/observer-service;1'].getService(Components.interfaces.nsIObserverService).notifyObservers(window, 'tabswitch-urlfile', this.value);"/>

Stock to watch? Probably should be something else.
Attachment #8621067 - Flags: review?(mconley)
Attachment #8621067 - Flags: review-
Attachment #8621067 - Flags: feedback?(avihpit)
Comment on attachment 8621067 [details] [diff] [review]
tps.patch

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

::: talos/pageswitch/bootstrap.js
@@ +224,5 @@
> +    var prefFile = prefs.getCharPref("addon.test.tabswitch.urlfile");
> +    if (prefFile) {
> +      handleFile(window, prefFile);
> +    } else {
> +      gTestURLs = ["http://mit.edu", "http://mozilla.org", "http://boston.com", "http://cnn.com", "http://bostonglobe.com", "http://nytimes.com", "http://cnet.com", "http://eff.org", "http://yahoo.com", "http://google.com"];

Do these get auto-proxied by Talos or loaded live over the network? (probably a question for jmaher)

We need to use cached versions of sites so we don't introduce noise/bustage from network issues or site changes.
oh, that shouldn't work in talos land- I was under the impression we were loading the tp5 pageset- maybe this list was what blassey used for local testing.
After discussing this on IRC with mconley, and without reading the actual code too deeply:

1. This should probably be a page-load test. These tests can report a single or multiple results to talos via the `tpRecordTime` function which the page-loader addon injects to the content.

2. It's probably better to report individual tab-switch times instead of an aggregation (sum or average) of them, since this way we can also identify regressions of switching to specific content, and the aggregation is still exposed by the system anyway (e.g. at graph server or compare talos etc). So reporting switch times for individual pages is a win-win. I don't recall the exact API but it allows reporting a list of tests (or pages or whatever) names and their numeric value.

3. mconley touched this as well - talos tests should not and cannot access the network. However, the entire talos repo folder is accessible via a local web server to any of the tests, so one would need to save some pages locally and add them to the repo.

Alternatively, you could pick some of the pages from the tp5o set (top 50 alexa pages which talos can access locally).

Regardless, you probably shouldn't declare the same array of URLs twice.

4. If you feel like it, it's usually very useful to be able to run talos tests outside of the talos framework - this way one could use profiling, or custom browser setups etc while testing (and without the need to setup talos locally). There's no "rule" how to do that but typically it's not too much hassle to make it work also outside of talos and display the results on screen instead of reporting them.

5. mconley asked which we should pick between Date.now() and performance.now(). Performance.now() is more accurate, however, its 'zero' is per content (tab?) so it might make it harder to measure time of tab switching. However, if you always use the same window for accessing performance.now() then it shouldn't be an issue.
Attachment #8621067 - Flags: feedback?(avihpit) → feedback-
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #3)
> Comment on attachment 8621067 [details] [diff] [review]
> tps.patch
> 
> Review of attachment 8621067 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: talos/pageswitch/bootstrap.js
> @@ +224,5 @@
> > +    var prefFile = prefs.getCharPref("addon.test.tabswitch.urlfile");
> > +    if (prefFile) {
> > +      handleFile(window, prefFile);
> > +    } else {
> > +      gTestURLs = ["http://mit.edu", "http://mozilla.org", "http://boston.com", "http://cnn.com", "http://bostonglobe.com", "http://nytimes.com", "http://cnet.com", "http://eff.org", "http://yahoo.com", "http://google.com"];
> 
> Do these get auto-proxied by Talos or loaded live over the network?
> (probably a question for jmaher)
> 
> We need to use cached versions of sites so we don't introduce noise/bustage
> from network issues or site changes.

Those don't get used at all. They are included (as is the options.xul) for local testing with the add-on absent the page set. The load time is also irrelevant as the test waits for all the pages to be loaded before starting the timer and switching tabs.
Attached patch tps.patch (obsolete) — Splinter Review
Attachment #8621067 - Attachment is obsolete: true
Attachment #8625314 - Flags: review?(mconley)
Attachment #8625314 - Flags: review?(avihpit)
(In reply to Mike Conley (:mconley) - Needinfo me! from comment #2)
> Comment on attachment 8621067 [details] [diff] [review]
> tps.patch
> @@ +183,5 @@
> > +  var localURI = Services.io.newFileURI(localFile);
> > +  req.open('get', localURI.spec, false);
> > +  req.send(null);
> > +  var parent = localURI.spec.split(localFile.leafName)[0];
> > +  var lines = req.responseText.split('<a href = \"');
> 
> This seems kinda fragile...
> 
> Dunno if this is any better, but maybe try:
> 
> win.document.createElement("body");
> body.innerHTML = req.responseText;
> let anchors = body.querySelectorAll("a");
> let links = [...anchors].map((anchor) => {
>   return anchor.getAttribute("href");
> });
> 
> links.forEach(function(link) {
>   gTestURLs.push(parent + link);
> });
> 
I had originally wanted to do the same thing (well.. getElementsByTagName instead of query selectors) with the responseXML. But, the tp5 index page isn't well formed, so it throws and even if you catch and handle that is generally useless.
I would be happy to change anything in tp5n.zip so we could have a proper index.
At the end of the test, it dumps "total tab switch time:14786.315629000033" to the console. I can add what ever decoration you need to parse that.
there are two types of tests in talos:
1) startup-tests
2) pageloader-tests

In this case we are dealing with a pageloader-test as we include the pageloader addon and depend on it and pagesets.  With this, we have a format for parsing results:
__start_tp_report
 _x_x_mozilla_page_load
 _x_x_mozilla_page_load_details
 |i|pagename|runs|
 |0;amazon.com/www.amazon.com/index.html;2386;1146
 |1;m.yahoo.co.jp/www.yahoo.co.jp/index.html;1724;901
 |2;m.accuweather.com/www.accuweather.com/index.html;228;231
 |3;m.yandex.ru/www.yandex.ru/index.html;6043;2984
 |4;m.wikipedia.com/en.m.wikipedia.org/index.html;734;385
 |5;m.espn.com/m.espn.go.com/index.html;576;419
 |6;m.bbc.co.uk/www.bbc.co.uk/mobile/index.html;349;229
 __end_tp_report
__start_cc_report
_x_x_mozilla_cycle_collect,3390
__end_cc_report
__startTimestamp1433438438092__endTimestamp


within other tests which use the pageloader, we just add results to it via the callback (http://hg.mozilla.org/build/talos/file/33449734f99f/talos/page_load_test/tart/addon/content/tart.js#l397):
content.tpRecordTime(csvTimes, csvNames)

Then as we do a set of iterations, pageloader will collect the list of times for each name and build the table.
Comment on attachment 8625314 [details] [diff] [review]
tps.patch

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

r-'ing just because I think we want to record the individual tab switch times, as avih suggested. Otherwise, this looks fine to me.

jmaher - what else would be required in order to get this test integrated? I assume we need to add an entry to test.py... I also assume we'd need to modify some databases somewhere to accept these numbers... what else? Also, please see my question regarding the parsing of TART results, as I don't 100% know how it's done.

::: talos/pageswitch/bootstrap.js
@@ +32,5 @@
> +  return deferred.promise;
> +}
> +
> +function executeSoon(callback) {
> +  Services.tm.mainThread.dispatch(callback, Components.interfaces.nsIThread.DISPATCH_NORMAL);

Just use Ci here.

@@ +80,5 @@
> +  waitForTabLoads(win.gBrowser, urls.length, function() {
> +    callback(win.gBrowser.getTabsToTheEndFrom(initialTab));
> +  });
> +  win.gBrowser.loadTabs(urls, true);
> +

Remove newline

@@ +99,5 @@
> +      if (index == tabs.length - 1) {
> +        callback();
> +      } else {
> +        runTestHelper(startTab, tabs, index + 1, win, function() {
> +          callback();

So I think what avih wants to do here is record a time for each individual tab switch - so can you use a Map or an Object to map the URI of the browser we just switched to (win.gBrowser.selectedBrowser.currentURI.spec) to the time it took to switch to it.

We might therefore also want to add a blank tab as the first time, so that we have a tab switch time for the first entry in the tabs list as well.

Then, once we have all of the time mappings in the Map / Object, blast them out via:

content.tpRecordTime(times.join(','), 0, tabUrls.join(','));

I _think_ that will be parseable by talos? At least, this is the way that TART seems to do it, so we might be able to re-use the same mechanism...

A although I don't exactly know what the parsing mechanism is. jmaher, are the TART values that get pumped out parsed server-side? Can we re-use the same mechanism here?

::: talos/pageswitch/install.rdf
@@ +14,5 @@
> +    <em:targetApplication>
> +      <Description>
> +        <em:id>{ec8030f7-c20a-464f-9b0e-13a3a9e97384}</em:id>
> +        <em:minVersion>38.0</em:minVersion>
> +        <em:maxVersion>41.0</em:maxVersion>

We might want to pump up this max version a bit. TART uses 99.0.*. We'll probably want to do something similar.
Attachment #8625314 - Flags: review?(mconley)
Attachment #8625314 - Flags: review?(avihpit)
Attachment #8625314 - Flags: review-
Attachment #8625314 - Flags: feedback?(jmaher)
yes, if we have a list of tabs and times related to them, it will magically get put into the correct format.  TART does this with the different measurements (load, error, etc.), we can do it with the specific pages for this test if that is what we want to measure.
(In reply to Joel Maher (:jmaher) from comment #13)
> yes, if we have a list of tabs and times related to them, it will magically
> get put into the correct format.  TART does this with the different
> measurements (load, error, etc.), we can do it with the specific pages for
> this test if that is what we want to measure.

Okay, yes, that sounds like the right thing to do.

So we will measure the time to switch between individual tabs.
Comment on attachment 8625314 [details] [diff] [review]
tps.patch

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

it would be nice if we could have this work more like tart/cart/damp/tresize where the addon is a landing page and we have a url switch to make it automated.  Likewise tooling for generating a .xpi file for local testing.  The key here is the pageset which if we just have a local install it would use live webpages, otherwise it would default to automation and the tp5 pageset.

That might be asking too much, if it is, lets not stress ourselves with it.

A new test requires:
* graph server support (we already have it in)
* an wiki entry here: https://wiki.mozilla.org/Buildbot/Talos/Tests
* full integration into talos (add an entry in test.py), ensure it runs with "talos -e `which firefox` -a tps --develop"
* testing on try server to ensure this runs on all platforms at least 20 times without crashing/issues
* stability of numbers (i.e. not bi/tri-modal)
Attachment #8625314 - Flags: feedback?(jmaher) → feedback+
blassey - are you good to add the entry to test.py and the wiki entry? If you're too swamped with other things, let me know and I'll drive it from here.
Flags: needinfo?(blassey.bugs)
Attached patch tps.patch (obsolete) — Splinter Review
Attachment #8625314 - Attachment is obsolete: true
Flags: needinfo?(blassey.bugs)
Attachment #8627416 - Flags: review?(mconley)
Attachment #8627416 - Flags: review?(jmaher)
Comment on attachment 8627416 [details] [diff] [review]
tps.patch

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

I think this is close from a talos integration standpoint, just not sure this really ran as expected.

::: talos/test.py
@@ +176,2 @@
>      """
> +    Tests the amount of time it takes to switch between tabs

I think you accidentally changed the classname here and the comment.

@@ +294,5 @@
>        - half: average interval over the 2nd half of the animation.
>        - all: average interval over all recorded intervals.
>      """
>      tpmanifest = '${talos}/page_load_test/tart/tart.manifest'
> +    extensions = '${talos}/pageswitch'

we should set tpmanifest to be ${talos}/pageswitch/tps.manifest

::: talos/ttest.py
@@ +306,5 @@
>              os.environ['MOZ_CRASHREPORTER'] = '1'
>          else:
>              os.environ['MOZ_CRASHREPORTER_DISABLE'] = '1'
>  
> +        os.environ['MOZ_DISABLE_NONLOCAL_CONNECTIONS'] = '0'

this needs to remain disabled- maybe for --develop we could consider ignoring it.
Attachment #8627416 - Flags: review?(jmaher) → review-
Attached patch tps.patch (obsolete) — Splinter Review
sorry, had some garbage in that previous patch
Attachment #8627416 - Attachment is obsolete: true
Attachment #8627416 - Flags: review?(mconley)
Attachment #8627921 - Flags: review?(mconley)
Attachment #8627921 - Flags: review?(jmaher)
Comment on attachment 8627921 [details] [diff] [review]
tps.patch

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

I am not an expert at the bootstrap.js stuff, but overall this is looking much better.

::: talos/pageswitch/bootstrap.js
@@ +53,5 @@
> +                                           "nsISupportsWeakReference"]),
> +
> +    onStateChange: function(aBrowser, aWebProgress, aRequest, aStateFlags, aStatus) {
> +      let loadedState = Ci.nsIWebProgressListener.STATE_STOP |
> +        Ci.nsIWebProgressListener.STATE_IS_NETWORK;

we don't want mozafterpaint?
Attachment #8627921 - Flags: review?(jmaher) → review+
(In reply to Joel Maher (:jmaher) from comment #20)
> Comment on attachment 8627921 [details] [diff] [review]
> tps.patch
> 
> Review of attachment 8627921 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I am not an expert at the bootstrap.js stuff, but overall this is looking
> much better.
> 
> ::: talos/pageswitch/bootstrap.js
> @@ +53,5 @@
> > +                                           "nsISupportsWeakReference"]),
> > +
> > +    onStateChange: function(aBrowser, aWebProgress, aRequest, aStateFlags, aStatus) {
> > +      let loadedState = Ci.nsIWebProgressListener.STATE_STOP |
> > +        Ci.nsIWebProgressListener.STATE_IS_NETWORK;
> 
> we don't want mozafterpaint?

No, this is waiting for the pages to all be loaded before starting the test. We won't get mozafterpaints for background tabs without other special backflips, and even if we do that we have no idea which paint represents the final paint.
thanks :blassey.  I have pushed to try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7064dcf6629c.  We will iron out the kinks and see what happens.  Depending on the run time we might need a new job (i.e. g3) and that will add a few days to the schedule for turning this on.  Either way we are getting closer and keeping up the momentum.
Comment on attachment 8627921 [details] [diff] [review]
tps.patch

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

r=me assuming a good try run. Stoked to see this in production.

::: talos/pageswitch/bootstrap.js
@@ +196,5 @@
> +  Services.obs.removeObserver(observer, "tabswitch-do-test");
> +}
> +
> +function handleFile(win, file) {
> +

Nit - remove newline

::: talos/pageswitch/content/test.html
@@ +11,5 @@
> +                   .rootTreeItem
> +                   .QueryInterface(Components.interfaces.nsIInterfaceRequestor)
> +                   .getInterface(Components.interfaces.nsIDOMWindow);
> +          Services.obs.notifyObservers(mainWindow, 'tabswitch-do-test', null);
> +          Services.obs.addObserver(function onStartup(aSubject, aTopic, aData) {

Should probably remove this observer once we get the notification.
Attachment #8627921 - Flags: review?(mconley) → review+
So jmaher's try push is failing out because I think we're attempting to get about:home snippets off the network... did we forget to disable something here?
Flags: needinfo?(jmaher)
we think it is a about:newtab thing and need to change it to about:blank.
Flags: needinfo?(jmaher)
(In reply to Joel Maher (:jmaher) from comment #25)
> we think it is a about:newtab thing and need to change it to about:blank.

Ok, cool. Out of curiosity, who is working on that bit?
Flags: needinfo?(jmaher)
blassey mentioned it in irc that he would, although I don't mind doing it.  Monday at the earliest- it would be easier for me to take a patch and test it on try instead of hacking- I could spend an hour or so on this easily.  :mconley, maybe you could jump in?
Flags: needinfo?(jmaher)
Attached patch test-fix.diff (obsolete) — Splinter Review
The problem is that window.OpenBrowserWindow opens up about:home, which attempts to download the snippets off of the network.

This patch causes us to open about:blank in a new window instead.
Attachment #8629512 - Flags: review?(jmaher)
Attached patch test-fix.diff (obsolete) — Splinter Review
Actually, in order for us to have permission to request the tp5 page set, we apparently need to use OpenBrowserWindow... so I just flip the pref for what page we browse to when opening a new window.

When I run the test locally, however, we seem to attempt to hit Facebook on the network, and we shut down again. :(

I have no idea where that's happening.
Attachment #8629512 - Attachment is obsolete: true
Attachment #8629512 - Flags: review?(jmaher)
Attachment #8629518 - Flags: review?(jmaher)
Attachment #8629518 - Flags: review?(jmaher) → review-
this patch does a few things:
1) fixes newtab to use about:blank
2) specifies pagecycles = 1, I couldn't get >1 to work, not sure if we want that or not
3) adds a tp5o.html file- currently we only test 49 pages on tp5o, I matched that and removed 2 which seem to contact facebook.com

I was able to see this running locally, we will see if this works on try, but I would like to get some other eyes on the changes I made.
Attachment #8629518 - Attachment is obsolete: true
Attachment #8630065 - Flags: review?(mconley)
Comment on attachment 8630065 [details] [diff] [review]
fix tabswitch to load pages properly

This is still kinda busted. Yes, numbers come back, but when I watch the test, none of the tabs are loading the pages properly - they're showing the "page not found" screen.
Attachment #8630065 - Flags: review?(mconley) → review-
Is the entire tp5n set run by any test? I was able to capture at least some of the pages (digg, for example) attempting to make contact with Facebook over the network. That strikes me as odd, and likely to break any of our existing talos tests (unless those tests are OK to touch the network).

If it's not expected that tp5n is supposed to touch the network, then we might have to alter tp5n somewhat.
Flags: needinfo?(jmaher)
the entire set we don't run, I am not sure if some of the other pages touch the network.   In my patch I comment out two pages from the pageset and those are the ones which touch facebook.com.  In that patch I left out a change to test.py so it wouldn't run locally, I will get that fixed up.
Flags: needinfo?(jmaher)
Attached patch tps.patch (obsolete) — Splinter Review
Attachment #8627921 - Attachment is obsolete: true
Attachment #8630065 - Attachment is obsolete: true
Attachment #8632307 - Flags: review?(mconley)
Attachment #8632307 - Flags: review?(jmaher)
Comment on attachment 8632307 [details] [diff] [review]
tps.patch

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

why do we have a .xpi file attached here?  I would rather generate it as we do tart/cart/tresize.

Also I had created a tp5o.html file in my previous patch, this uses the pages we run for tp5o and tp5o_scroll and ensures there is no network access.

the problem with my additional patch was that it didn't run at all on windows.

running this on try, I don't see any tests running successfully on linux or windows.  Please ensure that this runs locally and produces results.
Attachment #8632307 - Flags: review?(jmaher) → review-
Attachment #8630065 - Attachment is obsolete: false
(In reply to Joel Maher (:jmaher) from comment #35)
> Comment on attachment 8632307 [details] [diff] [review]
> tps.patch
> 
> Review of attachment 8632307 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> why do we have a .xpi file attached here?  I would rather generate it as we
> do tart/cart/tresize.
we're not using the XPI. its included in the patch for convenience (similar to the tools menu item to run the test), but doesn't need to be.
Attached patch Folded fixes (obsolete) — Splinter Review
Attachment #8630065 - Attachment is obsolete: true
Attachment #8632882 - Attachment is obsolete: true
Attached patch add tabswitch test to talos (obsolete) — Splinter Review
this patch takes all the other patches and rolls into one patch.  using some tricks on try server I kept hacking on this until it worked, so the last 5 to 6 green 'g1' jobs show the results:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=86eaab9e7713

Overall I think this is just about ready- lets review the code and turn it loose.
Attachment #8632307 - Attachment is obsolete: true
Attachment #8632884 - Attachment is obsolete: true
Attachment #8632307 - Flags: review?(mconley)
Attachment #8633417 - Flags: review?(mconley)
Comment on attachment 8633417 [details] [diff] [review]
add tabswitch test to talos

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

Looks good. Maybe one follow-up to file, unless you want to fix it right here.

::: talos/page_load_test/pageswitch/bootstrap.js
@@ +1,2 @@
> +// -*- Mode: js; tab-width: 2; indent-tabs-mode: nil; js2-basic-offset: 2; js2-skip-preprocessor-directives: t; -*-
> +

We seem to have called this thing "tabswitch", but it's held under a "pageswitch" directory. Sounds needlessly confusing. Let's rename this folder "tabswitch" instead.

@@ +112,5 @@
> +function test(window) {
> +  let win = window.OpenBrowserWindow();
> +  let testURLs;
> +
> +  NewTabURL.override("about:blank")

We should probably do this right from the start, when the tabswitch addon is installed, and then reset it with NewTabURL.reset() when uninstalled.

::: talos/page_load_test/tp5o.html
@@ +48,5 @@
> +<a href = "homeway.com.cn/www.hexun.com/index.html">link</a><br>
> +<a href = "youtube.com/www.youtube.com/music.html">link</a><br>
> +<a href = "people.com.cn/people.com.cn/index.html">link</a><br>
> +<-- these two access www.facebook.com for some reason, only in tabswitch -->
> +<ahref = "digg.com/digg.com/news/story/New_logo_for_Mozilla_Firefox_browser.html">link</a><br>

If we're going to go with this file, can we instead:

1) Clean up the HTML so no spaces on either side of the =
2) Use the querySelector thing I suggested to blassey
3) Properly comment out any anchors we don't want to use?

Happy to leave that as a follow-up.
Attachment #8633417 - Flags: review?(mconley) → review+
I know I could carry over the r+, but please have a quick look when you have time.
Assignee: blassey.bugs → jmaher
Attachment #8633417 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8633546 - Flags: review?(mconley)
Comment on attachment 8633546 [details] [diff] [review]
add tps to talos (3.0)

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

Alright, let's put the spaghetti in the machine.
Attachment #8633546 - Flags: review?(mconley) → review+
\o/

Filed bug 1183713 for the querySelectorAll follow-up.
Comment on attachment 8633555 [details] [diff] [review]
update talos.json to pick up latest talos changes (1.0)

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

Go go go!
Attachment #8633555 - Flags: review?(mconley) → review+
https://hg.mozilla.org/mozilla-central/rev/a3e9fb33046d
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
:blassey, can you add a definition to the wiki:
https://wiki.mozilla.org/Buildbot/Talos/Tests#tps
Flags: needinfo?(blassey.bugs)
done. let me know if it should be less breif
Flags: needinfo?(blassey.bugs)
that is just fine- if there are future questions that keep coming up we can document it in more detail.
You need to log in before you can comment on or make changes to this bug.