Closed Bug 1412357 Opened 6 years ago Closed 6 years ago

Make mozscreenshots use mochitest asserts and logging for better integration

Categories

(Testing :: mozscreenshots, defect)

Product:
Testing
Component:
mozscreenshots
Version:
Version 3
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox59 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: mill2540, Assigned: mill2540, Mentored)

References

Details

Attachments

(1 file)

Bug 1412357 - Added mochitest asserts + logging to TestRunner.jsm.
59 bytes, text/x-review-board-request
mconley
: review+
Details 
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20170926100259

Steps to reproduce:

Currently, the mozscreenshots extension does not use mochitest's logging utilities. Also, it does not use mochitest asserts to handle error checking.     



Expected results:

Mozscreenshots should use mochitests logging utilities and asserts for error checking. This should provide better and more consistent interfacing with other parts of mozilla's testing ecosystem
Mentor: jaws, mconley
Version: unspecified → Version 3
Assignee: nobody → mill2540
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Comment on attachment 8922891 [details]
Bug 1412357 - Added mochitest asserts + logging to TestRunner.jsm.

https://reviewboard.mozilla.org/r/194052/#review200138

Thanks Robin! See below.

::: commit-message-a0052:4
(Diff revision 2)
> +Bug 1412357 - Added mochitest asserts + logging to TestRunner.jsm.  r?mconley
> +
> +Still todo: spread this around so that Screenshots.jsm is also using it
> +Also: is the TestRunner.initTest(mochitest task) the best way to give TestRunner access to these features?

The other way I can think of is by accessing the _first_ browser window via:

```
Services.wm.getWindowEnumerator("navigator:browser").getNext();
```

and calling the functions on it - since that's where< I believe, Mochitest injects its functions.

::: browser/base/content/browser.css:444
(Diff revision 2)
> +.toolbox-tabs {
> +  background-color: red;
> +}

This looks like leftovers from something else.

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:68
(Diff revision 2)
> +
> +  /**
>     * Load specified sets, execute all combinations of them, and capture screenshots.
>     */
>    async start(setNames, jobName = null) {
> +    // TODO: testTask will break *all* the other tests

Not sure what this means, but it sounds ominous...

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:194
(Diff revision 2)
> -        log.error("Error loading set: " + setName);
> -        log.error(ex);
> +        // TODO: should this just be deleted? Do we want to intercept exceptions?
> +        this.testTask.info("Error loading set: " + setName);
> +        this.testTask.info(ex);

I _suspect_ Mochitest will already take care of exceptions "properly" (so the test will fail).  Can you confirm?

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:224
(Diff revision 2)
>    *
>    * @param {String[]} selectors - array of CSS selectors for relevant DOM element
>    * @return {Geometry.jsm Rect} Rect holding relevant x, y, width, height with padding
>    **/
>    _findBoundingBox(selectors, windowType) {
> -    // No selectors provided
> +    this.testTask.isnot(selectors.length, 0, "_findBoundingBox: selectors argument cannot be empty");

Nit: the message in the `is`, `isnot`, `ok` function should describe what is expected, and not be the message describing what is _not_ expected.

So, "\_findBoundingBox: selectors argument cannot be empty" should probably be:

"\_findBoundingBox: selectors argument is not empty"

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:245
(Diff revision 2)
>          element = selector();
>        } else {
>          element = browserWindow.document.querySelector(selector);
>        }
>  
> -      // Selector not found
> +      this.testTask.ok(element, `Selector ${selector} was not found`);

Same as above - this should be:

`Element for selector ${selector} was found`.

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:344
(Diff revision 2)
>      const finalSelectors = [];
>      for (const obj of combo) {
>        if (!windowType) {
>          windowType = obj.windowType;
> -      } else if (windowType !== obj.windowType) {
> -        log.warn("\tConfigurations with multiple window types are not allowed");
> +      } else {
> +        this.testTask.is(windowType, obj.windowType, "Configuration combos with multiple window types are not allowed")

"Configuration has a consistent window type"
Attachment #8922891 - Flags: review?(mconley) → review-
Comment on attachment 8922891 [details]
Bug 1412357 - Added mochitest asserts + logging to TestRunner.jsm.

https://reviewboard.mozilla.org/r/194052/#review200138

> Not sure what this means, but it sounds ominous...

This should be fixed now. I just had to go into the other files that used TestRunner.jsm and add the init code
Comment on attachment 8922891 [details]
Bug 1412357 - Added mochitest asserts + logging to TestRunner.jsm.

https://reviewboard.mozilla.org/r/194052/#review201458

Looks good, Robin. Just a few more questions.

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:50
(Diff revision 4)
> -    log.debug("init");
> +    // Can't get the testing task into this function because it is called before start. I think?
> +    // log.debug("init");

If we can't do this, then we should just remove these lines / dead code.

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:68
(Diff revision 4)
> +
> +  /**
>     * Load specified sets, execute all combinations of them, and capture screenshots.
>     */
>    async start(setNames, jobName = null) {
> +    // TODO: testTask will break *all* the other tests

You said that this should have been fixed now in [this comment](https://bugzilla.mozilla.org/show_bug.cgi?id=1412357#c5), so can we remove this comment now?

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm
(Diff revision 4)
>      for (const obj of combo) {
>        if (!windowType) {
>          windowType = obj.windowType;
> -      } else if (windowType !== obj.windowType) {
> -        log.warn("\tConfigurations with multiple window types are not allowed");
> +      } else {
> +        this.testTask.is(windowType, obj.windowType, "All configurations in the combo have a single window type");
> -        return;

Why was the early return removed?
Attachment #8922891 - Flags: review?(mconley) → review-
Comment on attachment 8922891 [details]
Bug 1412357 - Added mochitest asserts + logging to TestRunner.jsm.

https://reviewboard.mozilla.org/r/194052/#review201602

::: browser/tools/mozscreenshots/browser_screenshots.js:7
(Diff revision 4)
>  add_task(async function capture() {
> +  TestRunner.initTest(this);

Would it be possible for this to be in head.js instead in order to reduce boilerplate in each test?
Comment on attachment 8922891 [details]
Bug 1412357 - Added mochitest asserts + logging to TestRunner.jsm.

https://reviewboard.mozilla.org/r/194052/#review201920

Looks good! Good call MattN on putting the init inside head.js.

I'm curious about one last thing - see below.

::: browser/tools/mozscreenshots/head.js:31
(Diff revision 8)
>    info("Checking for mozscreenshots extension");
>    return new Promise((resolve) => {
>      AddonManager.getAddonByID("mozscreenshots@mozilla.org", function(aAddon) {
>        isnot(aAddon, null, "The mozscreenshots extension should be installed");
>        TestRunner = Cu.import("chrome://mozscreenshots/content/TestRunner.jsm", {}).TestRunner;
> +      TestRunner.initTest(self);

What is `this` in this scope? I wouldn't think we'd need the `self` alias here...
Attachment #8922891 - Flags: review?(mconley) → review-
Comment on attachment 8922891 [details]
Bug 1412357 - Added mochitest asserts + logging to TestRunner.jsm.

https://reviewboard.mozilla.org/r/194052/#review201920

> What is `this` in this scope? I wouldn't think we'd need the `self` alias here...

Neither did I. However, when I try to pass "this" into the initTest, I get null errors later. I think maybe the function scope is getting garbage collected or something.
Comment on attachment 8922891 [details]
Bug 1412357 - Added mochitest asserts + logging to TestRunner.jsm.

https://reviewboard.mozilla.org/r/194052/#review202300

::: browser/tools/mozscreenshots/head.js:28
(Diff revision 8)
>    let dir = chromeRegistry.convertChromeURL(chromeURL).QueryInterface(Ci.nsIFileURL).file;
>    await AddonManager.installTemporaryAddon(dir);
>  
>    info("Checking for mozscreenshots extension");
>    return new Promise((resolve) => {
>      AddonManager.getAddonByID("mozscreenshots@mozilla.org", function(aAddon) {

Change this line to:
> AddonManager.getAddonByID("mozscreenshots@mozilla.org", aAddon => {

to allow you to use `this`.
Comment on attachment 8922891 [details]
Bug 1412357 - Added mochitest asserts + logging to TestRunner.jsm.

https://reviewboard.mozilla.org/r/194052/#review201458

> Why was the early return removed?

I think I was thinking about asserts as exceptions, but looking at it now I realzed that they won't halt execution. Now I am wondering if this should be an assert at all. It might have been better they way it was before, since the combo should be skipped, but the other combos should be not be effected.
Comment on attachment 8922891 [details]
Bug 1412357 - Added mochitest asserts + logging to TestRunner.jsm.

https://reviewboard.mozilla.org/r/194052/#review202944

::: commit-message-a0052:3
(Diff revision 9)
> +Moved the call to TestRunner.initTest to head.js as per MattN's suggestion.
> +This suffers from failures when the browser_boundingbox.js test's try to check that failures occure correctly.

Nit: I don't think these lines from the commit message add much of value. Commit messages, as a rule, should talk about what's changing and why - but saying "I moved this here per reviewers suggestion" is less important than "I moved this here because of how it fixes problems X, Y and Z".

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:351
(Diff revision 9)
> +    if (rect) {
> +      return;
> +    }

Now I'm wondering why you're returning early here. We do want to find a rect, which is why you're calling `this.testTask.ok(rect`... why wouldn't we call \_onConfigurationReady once we had the rect?
Attachment #8922891 - Flags: review?(mconley) → review-
Comment on attachment 8922891 [details]
Bug 1412357 - Added mochitest asserts + logging to TestRunner.jsm.

https://reviewboard.mozilla.org/r/194052/#review204184

Looks good! I've taken the liberty of pushing this to try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=128f5cfa9d1bf4ea4d7ddc65490b04c05a8da48a

Assuming a green run, let's land this! Thanks!
Attachment #8922891 - Flags: review?(mconley) → review+
Depends on: 1403686
Just FYI :mconley: this shouldn't land until 1403686 does. The dependency got cleared because we tried to land 1403686 but it got backed out.
Comment on attachment 8922891 [details]
Bug 1412357 - Added mochitest asserts + logging to TestRunner.jsm.

https://reviewboard.mozilla.org/r/194052/#review204718

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:26
(Diff revision 10)
>  // Create a new instance of the ConsoleAPI so we can control the maxLogLevel with a pref.
>  // See LOG_LEVELS in Console.jsm. Common examples: "All", "Info", "Warn", & "Error".
>  const PREF_LOG_LEVEL = "extensions.mozscreenshots@mozilla.org.loglevel";
>  XPCOMUtils.defineLazyGetter(this, "log", () => {
>    let ConsoleAPI = Cu.import("resource://gre/modules/Console.jsm", {}).ConsoleAPI;
>    let consoleOptions = {
>      maxLogLevel: "info",
>      maxLogLevelPref: PREF_LOG_LEVEL,
>      prefix: "mozscreenshots",
>    };
>    return new ConsoleAPI(consoleOptions);
>  });

IIUC, this will no longer be used and should be removed.

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:323
(Diff revision 10)
> -      log.warn("\tskipped configuration: " + ex);
> +      this.testTask.info(`\tskipped configuration [ ${combo.map((e) => e.name).join(", ")} ]`);
> +      this.testTask.info(`\treason: ${ex.toString()}`);

Can you please file a follow-up to have the suite fail for browser/tools/mozscreenshots/preferences/browser_preferences.js which is currently throwing a TypeError? We should use `testTask.ok(false, …)` for that case to cause the job to fail.
Blocks: 1417603
Comment on attachment 8922891 [details]
Bug 1412357 - Added mochitest asserts + logging to TestRunner.jsm.

https://reviewboard.mozilla.org/r/194052/#review204718

> Can you please file a follow-up to have the suite fail for browser/tools/mozscreenshots/preferences/browser_preferences.js which is currently throwing a TypeError? We should use `testTask.ok(false, …)` for that case to cause the job to fail.

I'm not sure what you mean. Do you want the `testTeask.ok(false, ...)` to go in the catch statement?
Comment on attachment 8922891 [details]
Bug 1412357 - Added mochitest asserts + logging to TestRunner.jsm.

https://reviewboard.mozilla.org/r/194052/#review204718

> I'm not sure what you mean. Do you want the `testTeask.ok(false, ...)` to go in the catch statement?

Also, if I am understanding correctly, then this will result in combos showing as failing tests if they throw an exception when they are applied. This will cause problems for tests like `TabsInTitlebar.jsm`, which relies on rejected promises to indicate that some configs should be skipped conditionally.
(In reply to Robin Miller from comment #24)
> Comment on attachment 8922891 [details]
> Bug 1412357 - Added mochitest asserts + logging to TestRunner.jsm.
> 
> https://reviewboard.mozilla.org/r/194052/#review204718
> 
> > Can you please file a follow-up to have the suite fail for browser/tools/mozscreenshots/preferences/browser_preferences.js which is currently throwing a TypeError? We should use `testTask.ok(false, …)` for that case to cause the job to fail.
> 
> I'm not sure what you mean. Do you want the `testTeask.ok(false, ...)` to go
> in the catch statement?

yes

(In reply to Robin Miller from comment #25)
> Also, if I am understanding correctly, then this will result in combos
> showing as failing tests if they throw an exception when they are applied.
> This will cause problems for tests like `TabsInTitlebar.jsm`, which relies
> on rejected promises to indicate that some configs should be skipped
> conditionally.

We should only cause the test to fail for exceptions, not intentionally skipped tests. There should be a way to distinguish them.
Comment on attachment 8922891 [details]
Bug 1412357 - Added mochitest asserts + logging to TestRunner.jsm.

https://reviewboard.mozilla.org/r/194052/#review207012

Looks good! Just minor cosmetic stuff now.

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:42
(Diff revision 12)
>    },
>  
>    /**
> +   * Initialize the mochitest interface. This allows TestRunner to integrate
> +   * with mochitest functions like is(...) and ok(...). This must be called
> +   * prior to invoking any of the TestRunner functions. Note that this should 

Trailing whitespace

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:194
(Diff revision 12)
> -  * Calculate the bounding box based on CSS selector from config for cropping
> +   * Calculate the bounding box based on CSS selector from config for cropping
> -  *
> +   *
> -  * @param {String[]} selectors - array of CSS selectors for relevant DOM element
> +   * @param {String[]} selectors - array of CSS selectors for relevant DOM element
> -  * @return {Geometry.jsm Rect} Rect holding relevant x, y, width, height with padding
> +   * @return {Geometry.jsm Rect} Rect holding relevant x, y, width, height with padding
> -  **/
> +   **/

Thanks!

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:260
(Diff revision 12)
>  
>    async _performCombo(combo) {
>      let paddedComboIndex = padLeft(this.currentComboIndex + 1, String(this.combos.length).length);
> -    log.info("Combination " + paddedComboIndex + "/" + this.combos.length + ": " +
> -             this._comboName(combo).substring(1));
> +    this.mochitestScope.info(
> +        `Combination ${paddedComboIndex}/${this.combos.length}: ${this._comboName(combo).substring(1)}`
> +        );

Busted indentation

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:269
(Diff revision 12)
> -        setTimeout(reject, APPLY_CONFIG_TIMEOUT_MS, "Timed out");
> +          setTimeout(reject, APPLY_CONFIG_TIMEOUT_MS, "Timed out");
> -      });
> +          });

Busted indentation - should be 2 space.

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:276
(Diff revision 12)
> -        return new Promise((resolve) => {
> +          return new Promise((resolve) => {
> -          setTimeout(resolve, 500);
> +              setTimeout(resolve, 500);
> -        });
> +              });
> -      });
> +          });

Elsewhere, we're using 2-space indentation. Please try to use that throughout this patch.

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:351
(Diff revision 12)
>      const imagePath = await Screenshot.captureExternal(filename);
>  
>      let browserWindow = Services.wm.getMostRecentWindow("navigator:browser");
> -    await this._cropImage(browserWindow, OS.Path.toFileURI(imagePath), rect, imagePath);
> +    await this._cropImage(browserWindow, OS.Path.toFileURI(imagePath), rect, imagePath).catch((msg) => {
> +        throw `Cropping combo [${combo.map((e) => e.name).join(", ")}] failed: ${msg}`;
> +        });

Nit - busted indentation
Attachment #8922891 - Flags: review+ → review-
Comment on attachment 8922891 [details]
Bug 1412357 - Added mochitest asserts + logging to TestRunner.jsm.

https://reviewboard.mozilla.org/r/194052/#review207028

Looks good! Thanks, Robin!
Attachment #8922891 - Flags: review?(mconley) → review+
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/bc89bf67b38d
Added mochitest asserts + logging to TestRunner.jsm.  r=mconley
https://hg.mozilla.org/mozilla-central/rev/bc89bf67b38d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Depends on: 1425349
You need to log in before you can comment on or make changes to this bug.