Closed Bug 1313865 Opened 8 years ago Closed 8 years ago

Centralise type validation assertions in Marionette

Categories

(Remote Protocol :: Marionette, defect)

Product:
Remote Protocol
Component:
Marionette
Version:
Version 3
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox52 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox52 --- fixed

People

(Reporter: ato, Assigned: ato)

Details

Attachments

(4 files)

Bug 1313865 - Centralise common Marionette assertions;
58 bytes, text/x-review-board-request
automatedtester
: review+
impossibus
: review+
Details
Bug 1313865 - Employ common assertions in action module;
58 bytes, text/x-review-board-request
impossibus
: review+
Details
Bug 1313865 - Remove unused logger;
58 bytes, text/x-review-board-request
impossibus
: review+
Details
Bug 1313865 - Employ common assertions in GeckoDriver;
58 bytes, text/x-review-board-request
automatedtester
: review+
Details 
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: nobody → ato
Status: NEW → ASSIGNED
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 on attachment 8806152 [details]
Bug 1313865 - Centralise common Marionette assertions;

https://reviewboard.mozilla.org/r/89648/#review89416
Attachment #8806152 - Flags: review?(dburns) → 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 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 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 on attachment 8806152 [details]
Bug 1313865 - Centralise common Marionette assertions;

https://reviewboard.mozilla.org/r/89648/#review91264
Attachment #8806152 - Flags: review?(mjzffr) → 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 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 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 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+
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
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: