Closed Bug 1420733 Opened 7 years ago Closed 6 years ago

Indicate skipped configurations with return value instead of exceptions

Categories

(Testing :: mozscreenshots, enhancement)

Product:
Testing
Component:
mozscreenshots
Version:
Version 3
Type:
enhancement
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 1420733 - Adds support for skipping a configuration and correctly handling other exceptions. .
59 bytes, text/x-review-board-request
jaws
: review+
Details 
Currently, the mozscreenshots tool uses exceptions thrown in applyConfig to indicate that a test should be skipped, e.g. because it is not supported on a particular platform. Instead, configurations should be able to return an object from applyConfig indicating that the configuration should be skipped, and other exceptions should appear as non-fatal failed asserts.
Assignee: nobody → mill2540
Comment on attachment 8931947 [details]
Bug 1420733 - Adds support for skipping a configuration and correctly handling other exceptions.  .

https://reviewboard.mozilla.org/r/203008/#review208482

r- mainly for question about usefulness of `skip` argument.

::: commit-message-960f5:1
(Diff revision 2)
> +Bug 1420733 - Adds support for skipping a configuration and correctly handleing other exceptions.  r?jaws.

s/handleing/handling/

::: browser/tools/mozscreenshots/mozscreenshots/extension/Result.jsm:11
(Diff revision 2)
> +
> +this.EXPORTED_SYMBOLS = ["Result"];
> +
> +const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
> +
> +class Result {

Can you add some JSDoc for this class and constructor?

::: browser/tools/mozscreenshots/mozscreenshots/extension/Result.jsm:12
(Diff revision 2)
> +  constructor(reason, skip) {
> +    this.reason = reason;
> +    this.skip = skip || skip === undefined;

I don't see any files in this that are using skip=false (none explicitly set the `skip` argument). We should just remove it since it's unused.

If you do have a good reason to keep it, instead of checking if `skip === undefined`, you can set a default value for `skip`.

```js
constructor(reason, skip = true) {
  ...
}
```

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:321
(Diff revision 2)
> -      this.mochitestScope.info(`\treason: ${ex.toString()}`);
> -      // Don't set lastCombo here so that we properly know which configurations
> -      // need to be applied since the last screenshot
> -
> -      // Return so we don't take a screenshot.
> +      this.mochitestScope.info(`\t${ex}`);
> +      if (ex.stack) {
> +        for (const line of ex.stack.split("\n")) {
> +          this.mochitestScope.info(`\t${line}`);
> +        }

You don't need to split and iterate over the stack, you can just dump it as-is, like we do at https://searchfox.org/mozilla-central/rev/8839daefd69087d7ac2655b72790d3a25b6a815c/browser/base/content/test/about/head.js#15

I wouldn't worry too much about the tab indenting.

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/Buttons.jsm:13
(Diff revision 2)
>  
>  const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
>  
>  Cu.import("resource:///modules/CustomizableUI.jsm");
>  Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("chrome://mozscreenshots/content/Result.jsm");

Result.jsm isn't used by Button.jsm, does it need to be imported here?
Attachment #8931947 - Flags: review?(jaws) → review-
I'm not really sure we need to make this change.

Why can't we instead assume that a rejection reason that is a string (or not e.g. `instanceof Error`) is because the config should be skipped?

If that doesn't work then I'm also not sure that the Failure class is necessary. Could we use a primitive return value instead? Or a vanilla object? IMO `try` and `try_else` hide the flow of the code and the Cu.import adds boilerplate to configurations which I'd like to keep to a minimum.

For the latter issue you could pass the class to the methods and use destructuring to use it when needed.
Re: the need for Failure, I would be fine with using a vanilla object or returned string. The Failure class originally had some additional information attached to it, but that's not being used anymore.

However, I am not sure that using `instanceof Error` is going to work well in general. For example, one thing Jared and I were talking about is the case were a configuration containing a bug needs to be temporarily disabled. It might be nice if it could return something like { disabled: "Bug XXX" }, which would cause something like `Assert.todo(false, result.disabled)` to be run in TestRunner.
(In reply to Robin Miller from comment #6)
> For example, one thing Jared and I were talking about is the
> case were a configuration containing a bug needs to be temporarily disabled.
> It might be nice if it could return something like { disabled: "Bug XXX" },
> which would cause something like `Assert.todo(false, result.disabled)` to be
> run in TestRunner.

Returning a vanilla object like {disabled: "Bug XXX"} would be fine here.
Comment on attachment 8931947 [details]
Bug 1420733 - Adds support for skipping a configuration and correctly handling other exceptions.  .

https://reviewboard.mozilla.org/r/203008/#review210286

Clearing review so you can switch to using a vanilla JS object.
Attachment #8931947 - Flags: review?(jaws)
Attachment #8931947 - Flags: feedback?(mconley)
Attachment #8931947 - Flags: feedback?(jaws)
Attachment #8931947 - Flags: feedback?(mconley)
Attachment #8931947 - Flags: feedback?(jaws)
Comment on attachment 8931947 [details]
Bug 1420733 - Adds support for skipping a configuration and correctly handling other exceptions.  .

https://reviewboard.mozilla.org/r/203008/#review217904

r+ with the following changes. Thanks!

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:330
(Diff revision 5)
> -          await changeConfig(config);
> +          // I am wondering if I should move the try-catch block were, so that
> +          // the error message can be more specific

I am fine with leaving it where you have it now since it already logs the correct call stack.

::: browser/tools/mozscreenshots/mozscreenshots/extension/TestRunner.jsm:370
(Diff revision 5)
> -      // need to be applied since the last screenshot
> -
> +        this.mochitestScope.info(`\t${ex.stack}`);
> +      }
> -      // Return so we don't take a screenshot.
>        return;
>      }
> +    this.mochitestScope.ok(true, `Configured UI for [ ${combo.map(({ name }) => name).join(", ")} ] successfully`);

We should probably change this to info() since it will always be true.

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/Preferences.jsm:105
(Diff revision 5)
> +    // TODO: this fails because there is no such element
> +    // And the dialog may no longer exist
>      content.document.getElementById("doNotTrackSettings").click();

This can be changed to return `{todo: "The dialog may have exited before we could click the button"}` if `getElementById` doesn't find the element. Otherwise this can continue.

::: browser/tools/mozscreenshots/mozscreenshots/extension/configurations/Tabs.jsm:54
(Diff revision 5)
>          browserWindow.gBrowser.pinTab(tab);
>          browserWindow.gBrowser.selectTabAtIndex(5);
>          hoverTab(browserWindow.gBrowser.tabs[2]);
>          // also hover the new tab button
>          let newTabButton = browserWindow.document.getAnonymousElementByAttribute(browserWindow.
> -                           gBrowser.tabContainer, "class", "tabs-newtab-button");
> +                           gBrowser.tabContainer, "anonid", "tabs-newtab-button");

Thanks for fixing this.
Attachment #8931947 - Flags: review?(jaws) → review+
I retriggered the try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=8497e7e17a4e71ab6970ed16129df083888838c0&selectedJob=155827316 on OSX and it is continuously timing out on OSX due to bug 1425394. I propose that we disable the test on OSX.

MattN, are you OK with disabling the test on OSX?
Flags: needinfo?(MattN+bmo)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #13)
> I retriggered the try push at
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=8497e7e17a4e71ab6970ed16129df083888838c0&selectedJob=1
> 55827316 on OSX and it is continuously timing out on OSX due to bug 1425394.
> I propose that we disable the test on OSX.
> 
> MattN, are you OK with disabling the test on OSX?

No, we should instead disable the timeout like I did in bug 1425394 comment 24. It's expected that browser_primaryUI take longer than 1000 seconds (16+ minutes) so the timeout is just stopping the working job in progress. The fix to fourTabs in Tabs.jsm makes the job always take more than 16 minutes as it adds 50% more screenshots in primaryUI since fourTabs was permanently broken (bug 1427802).

I'll do a try push rebased on bug 1425394 comment 24 and confirm that it works. If so, I'll make that change for all platforms.
Flags: needinfo?(MattN+bmo)
That worked so I guess I can push this…
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8df2e3d57e94
Adds support for skipping a configuration and correctly handling other exceptions.  r=jaws.
https://hg.mozilla.org/mozilla-central/rev/8df2e3d57e94
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: