Closed Bug 1410210 Opened 3 years ago Closed 3 years ago

Remove talos pageloader command line options

Categories

(Testing :: Talos, enhancement)

Version 3
enhancement
Not set
normal

Tracking

(firefox59 fixed)

RESOLVED FIXED
mozilla59
Tracking Status
firefox59 --- fixed

People

(Reporter: rwood, Assigned: rwood)

References

Details

(Whiteboard: [PI:November])

Attachments

(1 file)

The talos pageloader add-on currently receives a bunch of options via command line [1]. Find a better way to pass this info to pageloader and remove these command line options.

[1] http://searchfox.org/mozilla-central/source/testing/talos/talos/pageloader/components/tp-cmdline.js
Assignee: nobody → rwood
Status: NEW → ASSIGNED
Whiteboard: [PI:November]
Comment on attachment 8926623 [details]
Bug 1410210 - Remove talos pageloader command line options;

https://reviewboard.mozilla.org/r/197850/#review203236

while I have some comments of future work, I would like to see some tests written for this- testing invalid values and how we error out in pageloader when receiving these.  The main difference here is the commandline would not accept many values, now we are passing in strings, ints, bools

::: testing/talos/talos/pageloader/chrome/pageloader.js:25
(Diff revision 1)
>  
>  var numRetries = 0;
>  var maxRetries = 3;
>  
>  var pageFilterRegexp = null;
>  var useBrowser = true;

how is useBrowser  different from the new variable useBrowserChrome added below?

::: testing/talos/talos/pageloader/chrome/pageloader.js:141
(Diff revision 1)
>  
> -    if (profilingInfo) {
> -      TalosParentProfiler.initFromObject(profilingInfo);
> +    NUM_CYCLES = Services.prefs.getIntPref("talos.tpcycles", 1);
> +    numPageCycles = Services.prefs.getIntPref("talos.tppagecycles", 1);
> +    noisy = Services.prefs.getBoolPref("talos.tpnoisy", false)
> +    timeout = Services.prefs.getIntPref("talos.tptimeout", -1)
> +    delay = Services.prefs.getIntPref("talos.tpdelay", 250)

can we get rid of:
* noisy
* timeout
* delay

::: testing/talos/talos/pageloader/chrome/pageloader.js:150
(Diff revision 1)
> +    scrollTest = Services.prefs.getBoolPref("talos.tpscrolltest", false)
> +    useBrowserChrome = Services.prefs.getBoolPref("talos.tpchrome", false)
> +
> +    // for pageloader tests the profiling info is found in an env variable
> +    // because it is not available early enough to set it as a browser pref
> +    var env = Components.classes["@mozilla.org/process/environment;1"].

interesting solution here, glad there was a way to solve this

::: testing/talos/talos/pageloader/chrome/pageloader.js:161
(Diff revision 1)
> +        TalosParentProfiler.initFromObject(JSON.parse(profilingInfo));
> +      }
>      }
>  
> -    forceCC = !args.noForceCC;
> -    doRenderTest = args.doRender;
> +    var tpNoForcecc = Services.prefs.getBoolPref("talos.tpnoforcecc", false);
> +    forceCC = !tpNoForcecc;

can we get rid of tpnoforcecc ?

::: testing/talos/talos/pageloader/chrome/pageloader.js:188
(Diff revision 1)
> -      dumpLine("tp: error: startIndex >= endIndex");
> -      plStop(true);
> -    }
>  
> +    var startIndex = 0;
> +    var endIndex = pages.length - 1;

do we need startIndex and endIndex as variables?  It sounds like we just need to delete these lines and the following line where we call pages.slice()

::: testing/talos/talos/pageloader/chrome/pageloader.js:206
(Diff revision 1)
>      var blank = Cc["@mozilla.org/supports-string;1"]
>        .createInstance(Ci.nsISupportsString);
>      blank.data = "about:blank";
>  
>      let toolbars = "all";
> -    if (!args.useBrowserChrome) {
> +    if (!useBrowserChrome) {

can we get rid of tpChrome and assume userBrowserChrome=False to simplify code?

::: testing/talos/talos/pageloader/components/tp-cmdline.js:80
(Diff revision 1)
> -      args.numCycles = cmdLine.handleFlagWithParam("tpcycles", false);
> -      args.numPageCycles = cmdLine.handleFlagWithParam("tppagecycles", false);
> -      args.startIndex = cmdLine.handleFlagWithParam("tpstart", false);
> -      args.endIndex = cmdLine.handleFlagWithParam("tpend", false);
> -      args.filter = cmdLine.handleFlagWithParam("tpfilter", false);
> -      args.useBrowserChrome = cmdLine.handleFlag("tpchrome", false);
> +    var tpmanifest = Services.prefs.getCharPref("talos.tpmanifest", null);
> +    if (tpmanifest == null) {
> +      // pageloader tests as well as startup tests use this pageloader add-on; pageloader tests
> +      // pass in a 'tpmanifest' which has been set to the talos.tpmanifest pref; startup tests
> +      // don't use 'tpmanifest' but pass in the test page which is forwarded onto the Firefox cmd
> +      // line. If this is a startup test exit here as we don't need a pageloader window etc.

possibly we can consider making pageloader an overlay that loads on start?  Also we could have a 'start' page we always load by default which could jumpstart pageloader and tell it to load prefs and start.
Attachment #8926623 - Flags: review?(jmaher) → review-
Comment on attachment 8926623 [details]
Bug 1410210 - Remove talos pageloader command line options;

https://reviewboard.mozilla.org/r/197850/#review203236

Thanks for the review, good call re: unit tests, I'll add those also

> how is useBrowser  different from the new variable useBrowserChrome added below?

Ah, removed

> can we get rid of:
> * noisy
> * timeout
> * delay

Noisy: tpnoisy is set to true by default in config.py, but I see that no tests change that setting. Ah yes, in the code if noisy is true it prints the "Cycle 1(X)..." line, I see no need to NOT have that line so yes I'll remove the option thx.

Delay: delay (between pageloads) wasn't even enabled on the cmd line to begin with but I wondered if it would be useful to keep. However you're right let's remove it since it's not actually being used.

Timeout: We need to keep, as you noted earlier on IRC tptimeout is used for ccov runs and added inside a tc transform.

> can we get rid of tpnoforcecc ?

Yes good call, I thought it might be a good feature to keep however it's not used anywhere and I see it's not even supported as a cmd line arg hah, I'll remove it.

> do we need startIndex and endIndex as variables?  It sounds like we just need to delete these lines and the following line where we call pages.slice()

Yes good call I'll remove them thanks

> can we get rid of tpChrome and assume userBrowserChrome=False to simplify code?

No, tpchrome is true by default and only *some* of the talos tests change it to false, so we need to keep it.

> possibly we can consider making pageloader an overlay that loads on start?  Also we could have a 'start' page we always load by default which could jumpstart pageloader and tell it to load prefs and start.

This might be an option except pageloader is used by startup tests (that just directly load a test page html and don't use pageloader manifests etc) as well as pageloader tests. I think we should consider this though but I'd like to land this cmd line cleanup initially and then consider further improvements down the road. :)
lets solve the removing of the tp-cmdline.js in a different bug and use this to cleanup the options, use prefs, and remove a lot of code :)
Duplicate of this bug: 1410211
(In reply to Robert Wood [:rwood] from comment #6)
> Comment on attachment 8926623 [details]
> Bug 1410210 - Remove talos pageloader command line options;
> 
> Review request updated; see interdiff:
> https://reviewboard.mozilla.org/r/197850/diff/1-2/

Update for review comments except the talos unit tests.
Comment on attachment 8926623 [details]
Bug 1410210 - Remove talos pageloader command line options;

https://reviewboard.mozilla.org/r/197850/#review203460

if you want to do unittests as a followup.. :)
Attachment #8926623 - Flags: review?(jmaher) → review+
Thanks! I may add the unit tests with this patch also, but initial try run anyway:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6d8ad1d7ebf00a6a278fed75a49da51f2414deca
Blocks: 1419534
Rebased patch (and some minor updates; carrying fw the r+). Landed on try again:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f8081d1fe74762cde4859859af23946fb9e5e1a8
Pushed by rwood@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a40ffd441b9a
Remove talos pageloader command line options; r=jmaher
https://hg.mozilla.org/mozilla-central/rev/a40ffd441b9a
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Depends on: 1420070
You need to log in before you can comment on or make changes to this bug.