Closed Bug 1423103 Opened 2 years ago Closed 2 years ago

Improve failure messages for invalid capability configurations

Categories

(Testing :: Marionette, enhancement, P3)

59 Branch
enhancement

Tracking

(firefox62 fixed)

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: whimboo, Assigned: akk5597, Mentored)

Details

(Keywords: good-first-bug, Whiteboard: [lang=js])

User Story

To get started please consult the following documentation for new contributors:

https://firefox-source-docs.mozilla.org/testing/marionette/marionette/NewContributors.html

Attachments

(1 file, 3 obsolete files)

This is similar to bug 1420577 but expands to all kinds of asserts we do in session.js, and which are related to session capabilities.

https://dxr.mozilla.org/mozilla-central/source/testing/marionette/session.js
Hi @whimboo
As you suggested, Can I start with this bug?
You are more than welcome! In case you have questions you can find me on IRC in the #ateam channel. Thanks.
Comment on attachment 8935706 [details]
Bug 1423103 - improved failure messages for invalid capability configurations

https://reviewboard.mozilla.org/r/206612/#review213380

::: testing/marionette/session.js:60
(Diff revision 1)
>        script: this.script,
>      };
>    }
>  
>    static fromJSON(json) {
> -    assert.object(json);
> +    assert.object(json, pprint `Expected an object, got ${json}`);

The failure message should make it clear what capability hasn't been specified correctly. Add the name similar to the entry names in the assert calls below.

::: testing/marionette/session.js:66
(Diff revision 1)
>      let t = new session.Timeouts();
>  
>      for (let [typ, ms] of Object.entries(json)) {
>        switch (typ) {
>          case "implicit":
> -          t.implicit = assert.positiveInteger(ms);
> +          t.implicit = assert.positiveInteger(ms,pprint `Expected entry "implicit" to be a positive integer, got ${ms}`);

Please add a space after the comma. As best run the linter via `mach lint testing/marionette` to make sure it all passes.

Also I would use `Expected entry for timeout "implicit"...`.

::: testing/marionette/session.js:294
(Diff revision 1)
>            [p.sslProxy, p.sslProxyPort] = fromHost("https", json.sslProxy);
>          }
>          if (typeof json.socksProxy != "undefined") {
>            [p.socksProxy, p.socksProxyPort] = fromHost("socks", json.socksProxy);
> -          p.socksVersion = assert.positiveInteger(json.socksVersion);
> +          p.socksVersion = assert.positiveInteger(json.socksVersion,
> +            pprint `Expected "socksVersion" to be a positive integer, got ${json.socksVersion}`);

nit: we use 4 chars of indentation in the files for wrapped lines. This also applies to all other changes in that file.

::: testing/marionette/session.js:427
(Diff revision 1)
>     */
>    static fromJSON(json) {
>      if (typeof json == "undefined" || json === null) {
>        json = {};
>      }
> -    assert.object(json);
> +    assert.object(json, pprint `Expected an object, got ${json}`);

Same as for the timeout object. Specify the name of what this refers to.

::: testing/marionette/session.js:440
(Diff revision 1)
>  
>      for (let [k, v] of Object.entries(json)) {
>        switch (k) {
>          case "acceptInsecureCerts":
> -          assert.boolean(v);
> +          assert.boolean(v,
> +            pprint `Expected "acceptInsecureCerts" to be boolean, got ${v}`);

To not duplicate the name of the capability you can just use `Expected "${k}" to be...`. Same for all the other checks inside of switch statements.
Attachment #8935706 - Flags: review?(hskupin) → review-
[Mass Change 2018-01-15] Moving bugs to backlog
Priority: -- → P3
Comment on attachment 8935706 [details]
Bug 1423103 - improved failure messages for invalid capability configurations

https://reviewboard.mozilla.org/r/206612/#review224222

::: commit-message-457b0:1
(Diff revision 2)
> +Bug 1423103 - made changes as requested in review

This is not a suitable commit message.

::: testing/marionette/session.js:60
(Diff revision 2)
>        script: this.script,
>      };
>    }
>  
>    static fromJSON(json) {
> -    assert.object(json);
> +    assert.object(json, pprint `Expected a session.Timeouts object, got ${json}`);

Throughout all your changes there should not be a space between "pprint" and the backtick character.

::: testing/marionette/session.js:67
(Diff revision 2)
>  
>      for (let [typ, ms] of Object.entries(json)) {
>        switch (typ) {
>          case "implicit":
> -          t.implicit = assert.positiveInteger(ms);
> +          t.implicit = assert.positiveInteger(ms,
> +              pprint `Expected entry for "implicit" to be a positive integer, got ${ms}`);

Can we shorten this to pprint`Expected implicit timeout duration to be a positive integer: ${ms}`?
Attachment #8935706 - Flags: review-
You need to combine these two patches into one.
Assignee: nobody → mohit2agrawal
Status: NEW → ASSIGNED
Comment on attachment 8935706 [details]
Bug 1423103 - improved failure messages for invalid capability configurations

https://reviewboard.mozilla.org/r/206612/#review224948

Sorry, but I cannot see that some of my issues have been addressed. Also you want to combine the commits into a single one via history rewriting. You can use `hg histedit` to do that.
Attachment #8935706 - Flags: review?(hskupin) → review-
Will you have time to clean up the patches and resubmit them?
Flags: needinfo?(mohit2agrawal)
Hi!
Can you please suggest what should be done for session.js: 60.
I am not able to figure out the correct message for it.
Thank you
Flags: needinfo?(mohit2agrawal)
I would propose: `Expected "session.Timeouts" to be an object, got ${json}`.
Attachment #8935706 - Attachment is obsolete: true
Comment on attachment 8949291 [details]
Bug 1423103 - improved failure messages for invalid capability configurations

https://reviewboard.mozilla.org/r/218674/#review228592

::: commit-message-8be22:5
(Diff revision 3)
> +Bug 1423103 - improved failure messages for invalid capability configurations r?whimboo
> +
> +MozReview-Commit-ID: LTWa8ckqe0j
> +***
> +Bug 1423103 - improved failure messages for invalid capability configurations

nit: the commit message still contains unwanted details. Please remove everything starting from "***".

::: testing/marionette/session.js:67
(Diff revision 3)
>  
>      for (let [typ, ms] of Object.entries(json)) {
>        switch (typ) {
>          case "implicit":
> -          t.implicit = assert.positiveInteger(ms);
> +          t.implicit = assert.positiveInteger(ms,
> +              pprint`Expected entry for "implicit" to be a positive integer, got ${ms}`);

Would you mind replacing removing `entry` here? That would align to other messages. 

So something like: `Expected "${typ}" timeout to be...`.

This also affects the other two case blocks.

::: testing/marionette/session.js:443
(Diff revision 3)
>  
>      for (let [k, v] of Object.entries(json)) {
>        switch (k) {
>          case "acceptInsecureCerts":
> -          assert.boolean(v);
> +          assert.boolean(v,
> +              pprint`Expected entry for "${k}" to be boolean, got ${v}`);

`Expected "${k}"...` please. Same for the case below.

::: testing/marionette/session.js:476
(Diff revision 3)
>            matched.set("timeouts", timeouts);
>            break;
>  
>          case "moz:webdriverClick":
> -          assert.boolean(v);
> +          assert.boolean(v,
> +              pprint`Expected entry for "${k}" to be boolean, got ${v}`);

Same as above. Remove `entry for` in both following cases.
Comment on attachment 8949291 [details]
Bug 1423103 - improved failure messages for invalid capability configurations

https://reviewboard.mozilla.org/r/218674/#review228594

We are getting close. Please fix the remaining wording issues and we should be fine to get it landed, or better to run another try job first.
Attachment #8949291 - Flags: review?(hskupin) → review-
Comment on attachment 8949291 [details]
Bug 1423103 - improved failure messages for invalid capability configurations

https://reviewboard.mozilla.org/r/218674/#review228592

> Would you mind replacing removing `entry` here? That would align to other messages. 
> 
> So something like: `Expected "${typ}" timeout to be...`.
> 
> This also affects the other two case blocks.

It looks like you missed to address this issue.
Comment on attachment 8949291 [details]
Bug 1423103 - improved failure messages for invalid capability configurations

https://reviewboard.mozilla.org/r/218674/#review229032
Attachment #8949291 - Flags: review?(hskupin)
Really missed that one.
Hope my future contributions would not take this much time.
Don't worry about it. It's all fine. The first patch is always the hardest.
Mohit, will you be able to finish the remaining work? Note, you might have to rebase against mozilla-central given that the code might have changed meanwhile.
Flags: needinfo?(mohit2agrawal)
Assignee: mohit2agrawal → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(mohit2agrawal)
Hey @whimboo,

As you suggested, I will start working on this bug, and put a review as soon as I can.
Attachment #8982235 - Attachment is obsolete: true
Attachment #8982235 - Flags: review?(hskupin)
Comment on attachment 8982400 [details]
Bug 1423103 - Improved failure messages for invalid capability configurations

https://reviewboard.mozilla.org/r/248344/#review254596

Thank you for the patch! Since the last time I had a look a bit has been changed in our code. So please have a look at the review comments, and get those implemented.

It's great to see that you even run the tests and fixed expectations. Thanks!

::: testing/marionette/session.js:59
(Diff revision 1)
>      };
>    }
>  
>    static fromJSON(json) {
> -    assert.object(json);
> +    assert.object(json,
> +        pprint`Expected "session.Timeouts" to be an object, got ${json}`);

nit: I wouldn't leak our internal structure to outside users of geckodriver/Marionette. I think it will be totally fine to use use `Expected "timeouts" ...`.

::: testing/marionette/session.js:62
(Diff revision 1)
>    static fromJSON(json) {
> -    assert.object(json);
> +    assert.object(json,
> +        pprint`Expected "session.Timeouts" to be an object, got ${json}`);
>      let t = new session.Timeouts();
>  
>      for (let [typ, ms] of Object.entries(json)) {

Please rename s/typ/type/

::: testing/marionette/session.js:66
(Diff revision 1)
>  
>      for (let [typ, ms] of Object.entries(json)) {
>        switch (typ) {
>          case "implicit":
> -          t.implicit = assert.positiveInteger(ms);
> +          t.implicit = assert.positiveInteger(ms,
> +              pprint`Expected ${typ} to be a positive integer, got ${ms}`);

Please make sure to add quotes around `${type}` in all the cases.

::: testing/marionette/session.js:431
(Diff revision 1)
>    static fromJSON(json) {
>      if (typeof json == "undefined" || json === null) {
>        json = {};
>      }
> -    assert.object(json);
> +    assert.object(json,
> +        pprint`Expected "session.Capabilities" to be an object, got ${json}"`);

Similar as above for timeouts. Just use `"capabilities"`.

::: testing/marionette/session.js:444
(Diff revision 1)
>  
>      for (let [k, v] of Object.entries(json)) {
>        switch (k) {
>          case "acceptInsecureCerts":
> -          assert.boolean(v);
> +          assert.boolean(v,
> +              pprint`Expected ${k} to be a boolean, got ${v}`);

Check for quotes in all the cases below.
Attachment #8982400 - Flags: review?(hskupin) → review-
Assignee: nobody → akk5597
Status: NEW → ASSIGNED
Comment on attachment 8982400 [details]
Bug 1423103 - Improved failure messages for invalid capability configurations

https://reviewboard.mozilla.org/r/248344/#review254596

I fixed the issues, and also retested and changes were needed in test_session.js and those have been done too.
Comment on attachment 8982400 [details]
Bug 1423103 - Improved failure messages for invalid capability configurations

https://reviewboard.mozilla.org/r/248344/#review254966

Code-wise this looks fine, but please have a look at the inline comment in regards linting. We might probably also see this failure in the try push which I just submitted. You can see the results via the link inside of the mozreview review request.

::: testing/marionette/test/unit/test_session.js:70
(Diff revisions 1 - 2)
>    try {
>      session.Timeouts.fromJSON({script: "foobar"});
>    } catch (e) {
>      equal(e.name, InvalidArgumentError.name);
> -    equal(e.message, "Expected [object String] \"script\" to be a positive integer, got [object String] \"foobar\"");
> +    equal(e.message, "Expected \"[object String] \"script\"\" to be a positive integer, got [object String] \"foobar\"");
>    }

The line length here should exceed the maximum allowed length for lines in .js files. The linter should have complained about. Did you run `mach lint testing/marionette`?
Attachment #8982400 - Flags: review?(hskupin) → review+
(In reply to Henrik Skupin (:whimboo) from comment #31)
> > -    equal(e.message, "Expected [object String] \"script\" to be a positive integer, got [object String] \"foobar\"");
> > +    equal(e.message, "Expected \"[object String] \"script\"\" to be a positive integer, got [object String] \"foobar\"");
> >    }
> 
> The line length here should exceed the maximum allowed length for lines in
> .js files. The linter should have complained about. Did you run `mach lint
> testing/marionette`?

So the Linter actually doesn't fail, but inspecting the format of the exception message again, I don't feel that confident now in having a double quote for the expected value. As it looks like we don't need the quotes in those cases when `pprint` is used.

Andreas, mind giving your input here? Do you agree?
Flags: needinfo?(ato)
Comment on attachment 8982400 [details]
Bug 1423103 - Improved failure messages for invalid capability configurations

https://reviewboard.mozilla.org/r/248344/#review255012

::: testing/marionette/session.js:66
(Diff revision 2)
>  
> -    for (let [typ, ms] of Object.entries(json)) {
> -      switch (typ) {
> +    for (let [type, ms] of Object.entries(json)) {
> +      switch (type) {
>          case "implicit":
> -          t.implicit = assert.positiveInteger(ms);
> +          t.implicit = assert.positiveInteger(ms,
> +              pprint`Expected "${type}" to be a positive integer, got ${ms}`);

I don’t think it makes sense to encapsulate the type variable with
quotes here because the error messages end up looking like this:

> Expected "[object String] "script"" to be a positive integer, got [object String] "foobar"

pprint will automatically output the string with quotes, so it is
unnecessary to do it in the template.  It is also grammatically
incorrect.
Attachment #8982400 - Flags: review-
Comment on attachment 8982400 [details]
Bug 1423103 - Improved failure messages for invalid capability configurations

https://reviewboard.mozilla.org/r/248344/#review254966

> The line length here should exceed the maximum allowed length for lines in .js files. The linter should have complained about. Did you run `mach lint testing/marionette`?

Strings aren’t affected by the line length check.
Flags: needinfo?(ato)
Comment on attachment 8982400 [details]
Bug 1423103 - Improved failure messages for invalid capability configurations

https://reviewboard.mozilla.org/r/248344/#review255012

> I don’t think it makes sense to encapsulate the type variable with
> quotes here because the error messages end up looking like this:
> 
> > Expected "[object String] "script"" to be a positive integer, got [object String] "foobar"
> 
> pprint will automatically output the string with quotes, so it is
> unnecessary to do it in the template.  It is also grammatically
> incorrect.

I've removed the quotes and also removed them from the test file for error messages.
Comment on attachment 8982400 [details]
Bug 1423103 - Improved failure messages for invalid capability configurations

https://reviewboard.mozilla.org/r/248344/#review255012

> I've removed the quotes and also removed them from the test file for error messages.

It looks fine. Please close the issue given that I don't have permission to do so. Once done we can get the patch landed. Thanks.
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e7e26022a370
Improved failure messages for invalid capability configurations r=whimboo
Comment on attachment 8982400 [details]
Bug 1423103 - Improved failure messages for invalid capability configurations

https://reviewboard.mozilla.org/r/248344/#review255520
Attachment #8982400 - Flags: review+
Ah, I thought I had to revoke my r-.  Apparently mozreview completely
disregards that someone had previously given the patch r-.
https://hg.mozilla.org/mozilla-central/rev/e7e26022a370
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Attachment #8949291 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.