Closed Bug 1437584 Opened 2 years ago Closed 2 years ago

Enable ESLint rule mozilla/use-services for testing/

Categories

(Testing :: General, enhancement)

Version 3
enhancement
Not set

Tracking

(firefox60 fixed)

RESOLVED FIXED
mozilla60
Tracking Status
firefox60 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

Details

Attachments

(1 file)

As we've been doing elsewhere, this continues the work to replace .getService() with Services.jsm uses where possible and enable the mozilla/use-services rule, this has generally more easy to read code and will make the testing code more consistent with the rest of development.
Joel, I'm trying to get this patch to pass on try, but I'm having some issues on Talos:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3797be6d18ac06251531ba533db9b47c5ed1f1eb

I'm guessing some of the files don't like accessing services, but I don't know which. I tried the tresize addon locally, but it worked fine, so its probably something else, however, I can't see any way to get console logs or how to debug what's going on?

Can you provide some suggestions or help please?
Flags: needinfo?(jmaher)
what is failing is non pageloader tests, so ts_paint, tresize specifically.  It could be changes in:
testing/talos/talos/scripts/MozillaFileLogger.js
testing/talos/talos/scripts/profiler.js
testing/talos/talos/startup_test/tresize/addon/content/Profiler.js
testing/talos/talos/startup_test/tspaint_test.html
testing/talos/talos/talos-powers/chrome/talos-powers-content.js 	

there is no trick to get more data from a talos run, I would suggest running it locally:
./mach talos-test -a tresize

Since it is failing early on, I would expect some kind of exception that doesn't get print to the stdout/stderr, possibly you could see it in the console.  Worse case, bisect the changes on the above files locally or on try.
Flags: needinfo?(jmaher)
Attachment #8950724 - Flags: review?(jmaher)
There's a full try push linked from mozreview, afaict none of the failures there are persistent / would be caused by this.
Comment on attachment 8950724 [details]
Bug 1437584 - Enable ESLint rule mozilla/use-services for testing/.

https://reviewboard.mozilla.org/r/219974/#review227398

a few questions/nits- otherwise this looks good

::: testing/specialpowers/content/SpecialPowersObserver.jsm:26
(Diff revision 4)
>  const CHILD_SCRIPT_API = "chrome://specialpowers/content/specialpowersAPI.js";
>  const CHILD_LOGGER_SCRIPT = "chrome://specialpowers/content/MozillaLogger.js";
>  
>  
>  // Glue to add in the observer API to this object.  This allows us to share code with chrome tests
> -var loader = Components.classes["@mozilla.org/moz/jssubscript-loader;1"]
> +Services.scriptloader.loadSubScript("chrome://specialpowers/content/SpecialPowersObserverAPI.js");

could we use CHILD_SCRIPT_API here as it is defined above?

::: testing/talos/talos/scripts/MozillaFileLogger.js:123
(Diff revision 4)
>  try {
> +  // Cu / ChromeUtils is not available in this scope.
> +  // eslint-disable-next-line mozilla/use-services
>    var prefs = Components.classes["@mozilla.org/preferences-service;1"]
>      .getService(Components.interfaces.nsIPrefBranch);
>    var filename = prefs.getCharPref("talos.logfile");

why not use something like:
var filename = Services.prefs.getCharPref("talos.logfile");

::: testing/talos/talos/startup_test/tspaint_test.html:44
(Diff revision 4)
>  function dumpConsoleAndQuit() {
>    var messages = {};
>  
>    try {
>      netscape.security.PrivilegeManager.enablePrivilege("UniversalXPConnect");
> +    // eslint-disable-next-line mozilla/use-services

could we use Services.console ?
Attachment #8950724 - Flags: review?(jmaher) → review+
Comment on attachment 8950724 [details]
Bug 1437584 - Enable ESLint rule mozilla/use-services for testing/.

https://reviewboard.mozilla.org/r/219974/#review227398

> could we use CHILD_SCRIPT_API here as it is defined above?

It is a different script being loaded so we can't.

> could we use Services.console ?

I thought it was harder to get that it actually was. I've fixed that now.
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a8300f975833
Enable ESLint rule mozilla/use-services for testing/. r=jmaher
https://hg.mozilla.org/mozilla-central/rev/a8300f975833
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.