Centralise type validation assertions in Marionette

RESOLVED FIXED in Firefox 52

Status

Testing
Marionette
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: ato, Assigned: ato)

Tracking

Version 3
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

(Assignee)

Description

a year ago
We make a lot of different assertions in Marionette that could be shared in a single module.  Instead of typeof obj == "boolean" and similar checks we should ensure such functionality is generally available.
(Assignee)

Updated

a year ago
Assignee: nobody → ato
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 5

a year ago
mozreview-review
Comment on attachment 8806155 [details]
Bug 1313865 - Employ common assertions in GeckoDriver;

https://reviewboard.mozilla.org/r/89654/#review89070
Attachment #8806155 - Flags: review?(dburns) → review+

Comment 6

a year ago
mozreview-review
Comment on attachment 8806152 [details]
Bug 1313865 - Centralise common Marionette assertions;

https://reviewboard.mozilla.org/r/89648/#review89416
Attachment #8806152 - Flags: review?(dburns) → review+

Comment 7

a year ago
mozreview-review
Comment on attachment 8806152 [details]
Bug 1313865 - Centralise common Marionette assertions;

https://reviewboard.mozilla.org/r/89648/#review89794

Yay for consolidating!
A few small issues.

::: testing/marionette/assert.js:130
(Diff revision 1)
> + *
> + * @return {number}
> + *     |obj| is returned unaltered.
> + *
> + * @throws {InvalidArgumentError}
> + *     If |obj| is not a signed integer.

This and the name sint don't match the actual behaviour. What about "not a positive integer"?

::: testing/marionette/error.js:137
(Diff revision 1)
> + * Pretty-print values passed to template strings.
> + *
> + * Usage:
> + *
> + *     let input = {value: true};
> + *     stringify`Expected boolean, got ${input}`;

pprint`Expected boolean...`;

::: testing/marionette/test_assert.js:30
(Diff revision 1)
> +});
> +
> +add_test(function test_defined() {
> +  assert.defined({});
> +  Assert.throws(() => assert.defined(undefined),
> +      InvalidArgumentError, /Expected [object Undefined] to be defined/);

The regex args here and elsewhere are just used as messages in the test run; they don't play a role in checking the raised exception. One could change this to `/whatever/` and the test would still pass. Is that what you intend?
Attachment #8806152 - Flags: review?(mjzffr) → review-

Comment 8

a year ago
mozreview-review
Comment on attachment 8806153 [details]
Bug 1313865 - Employ common assertions in action module;

https://reviewboard.mozilla.org/r/89650/#review89834

There are failures in test_action.js because some error messages have changed. Like: `check(/Expected boolean \(primary\)/, message, parametersData);`
Attachment #8806153 - Flags: review?(mjzffr) → review-

Comment 9

a year ago
mozreview-review
Comment on attachment 8806154 [details]
Bug 1313865 - Remove unused logger;

https://reviewboard.mozilla.org/r/89652/#review89836
Attachment #8806154 - Flags: review?(mjzffr) → review+
(Assignee)

Comment 10

a year ago
mozreview-review-reply
Comment on attachment 8806152 [details]
Bug 1313865 - Centralise common Marionette assertions;

https://reviewboard.mozilla.org/r/89648/#review89794

> The regex args here and elsewhere are just used as messages in the test run; they don't play a role in checking the raised exception. One could change this to `/whatever/` and the test would still pass. Is that what you intend?

Good catch!
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 15

a year ago
mozreview-review
Comment on attachment 8806152 [details]
Bug 1313865 - Centralise common Marionette assertions;

https://reviewboard.mozilla.org/r/89648/#review91264
Attachment #8806152 - Flags: review?(mjzffr) → review+

Comment 16

a year ago
mozreview-review
Comment on attachment 8806153 [details]
Bug 1313865 - Employ common assertions in action module;

https://reviewboard.mozilla.org/r/89650/#review91266

::: testing/marionette/test_action.js:41
(Diff revision 2)
>    let message = `parametersData: [pointerType: ${parametersData.pointerType}, ` +
>        `primary: ${parametersData.primary}]`;
>    check(/Unknown pointerType/, message, parametersData);
>    parametersData.pointerType = "pen";
>    parametersData.primary = "a";
>    check(/Expected boolean \(primary\)/, message, parametersData);

There are failures in test_action.js because some error messages have changed. This is one example.
Attachment #8806153 - Flags: review?(mjzffr) → review-

Comment 17

a year ago
mozreview-review-reply
Comment on attachment 8806153 [details]
Bug 1313865 - Employ common assertions in action module;

https://reviewboard.mozilla.org/r/89650/#review89834

I think this was missed, so I've rewritten it as an issue now to make it more visible.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 22

11 months ago
mozreview-review-reply
Comment on attachment 8806153 [details]
Bug 1313865 - Employ common assertions in action module;

https://reviewboard.mozilla.org/r/89650/#review91266

> There are failures in test_action.js because some error messages have changed. This is one example.

Ah, thanks for pointing this out!  This is probably what I erranously reported as a bug the other week.

Comment 23

11 months ago
mozreview-review
Comment on attachment 8806153 [details]
Bug 1313865 - Employ common assertions in action module;

https://reviewboard.mozilla.org/r/89650/#review91982
Attachment #8806153 - Flags: review?(mjzffr) → review+

Comment 24

11 months ago
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/84038d989111
Centralise common Marionette assertions; r=automatedtester,maja_zf
https://hg.mozilla.org/integration/autoland/rev/9455d3577d5b
Employ common assertions in action module; r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/0172122a6c43
Remove unused logger; r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/f8081bbfdf4e
Employ common assertions in GeckoDriver; r=automatedtester

Comment 25

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/84038d989111
https://hg.mozilla.org/mozilla-central/rev/9455d3577d5b
https://hg.mozilla.org/mozilla-central/rev/0172122a6c43
https://hg.mozilla.org/mozilla-central/rev/f8081bbfdf4e
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.