1163038 - test_gfxBlacklist* xpcshell tests pass locally without running

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Milan Sreckovic [:milan] (needinfo for best results)]

From what I can tell, this is common in all the test_gfxBlacklist*.js xpcshell tests:

  try {
    var gfxInfo = Cc["@mozilla.org/gfx/info;1"].getService(Ci.nsIGfxInfo);
  } catch (e) {
    do_test_finished();
    return;
  }

which always throws when run locally, so we quietly finish the test and pass without actually ever testing anything.

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

Ehsan, Jeff volunteered your name when xpcshell tests are in question :)

I see other tests that successfully get the gfx/info (test_location.js under search), but not these.  Running under debug, using Ci.nsIGfxInfo, Ci.nsIGfxInfo2 or Ci.nsIGfxInfoDebug all work for test_location.js, but not for any of the test_gfxBlacklist*.js ones.

Comment 2

[(no longer active)]

Is this one example? <https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpcshell/test_gfxBlacklist_OK.js>

From a quick look, it looks like nothing in that test defines Cc and Ci, and I bet we throw because of that.  Can you try adding the following to the beginning of this file and see if it fixes the issue?

const Cc = Components.classes;
const Ci = Components.interfaces;

Also, I'm not sure why that line is wrapped in a try/catch block.  Is that code ever expected to throw?  If not, you should make sure it's not wrapped in a try/catch block, otherwise failures such as this will be silently ignored.

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

Yes, that's one example.  Cc and Ci are defined somewhere, and replacing them with the long form work the same.  This part works:

var s = Cc["@mozilla.org/gfx/info;1"];

but then this part fails:

s.getService(Ci.nsIGfxInfo);

The same kind of code works just fine in other places (e.g., https://dxr.mozilla.org/mozilla-central/source/toolkit/components/search/tests/xpcshell/test_location.js#42), so it appears something is not initialized properly - was wondering if it was obvious to you as to what that was.

Comment 4

[(no longer active)]

Spoke with Milan about this in person.  There are other occurrences of Cc and Ci in these tests, and I suggested adding the const definitions to a head.js file.

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

The head file for this directory is head_addons.js, and doing something like:
const { classes: Cc, interfaces: Ci, utils: Cu, results: Cr } = Components;
in that file gets us further in these tests.

Comment 6

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: test_gfxBlocklist_OK.js test passing

Change the .xml OSX version to 10.6, we don't really support 10.5 in the code.

Comment 7

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Part 1. test_gfxBlocklist_OK.js test passing. r=me (Ehsan's code)

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

As we fix all of them, we can then move the repeated code from individual tests to head_addon.js file.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b77ba04e3d87

Comment 10

[Ekanan Ketunuti]

https://hg.mozilla.org/mozilla-central/rev/b77ba04e3d87

Comment 11

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Part 2. Most of the rest of the test_gfxBlacklist*js files. r=kats

Kats, I know this isn't necessarily what you've reviewed before, but it's pretty standard stuff.

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8636195 [details] [diff] [review]
Part 2. Most of the rest of the test_gfxBlacklist*js files. r=kats

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

::: toolkit/mozapps/extensions/test/xpcshell/test_gfxBlacklist_prefs.js
@@ +105,5 @@
>      do_execute_soon(ensureBlacklistUnset);
>    }
>    function ensureBlacklistUnset()
>    {
> +    mapFile("/data/test_gfxBlacklist.xml", gTestserver);

This one seems misplaced to me, but I might be misreading the test. Seems like the way it is in current m-c, it does a mapFile at the start of the test (line 14). Then it loads the mapped file (line 131) and that triggers the various test functions to run in sequence. As part of that sequence it maps and loads the other xml file (line 99).

Your change makes it so that when the first load_blocklist is called on line 131 the file hasn't been mapped yet; it only gets mapped later in the sequence, when ensureBlacklistUnset is called here.

Comment 13

[Milan Sreckovic [:milan] (needinfo for best results)]

You're right, the test_gfxBlacklist_prefs.js is not supposed to be part of this patch, this was a partial edit...

Comment 14

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: Part 2. Most of the rest of the test_gfxBlacklist*js files. Carry r=kats

The actual patch, without the _prefs.js file that wasn't meant to be in, and with minor changes, so I'm carrying r+.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=575f5062bc37

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d962d014dda5

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d962d014dda5