1420577 - Improve failure messages for invalid proxy configurations

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

As seen on https://github.com/mozilla/geckodriver/issues/1022#issuecomment-345784282 we fail with:

> "InvalidArgumentError: Expected [object Undefined] undefined to be an integer"

Those messages should include what specifically was wrong. The fix here should apply to probably all the checks we do.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1420577 - Improve failure messages for invalid proxy configurations.

Review commit: https://reviewboard.mozilla.org/r/205236/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/205236/

Comment 2

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8934315 [details]
Bug 1420577 - Improve failure messages for invalid proxy configurations.

https://reviewboard.mozilla.org/r/205236/#review210820

::: testing/marionette/session.js:277
(Diff revision 1)
> +            "Expected \"proxyAutoconfigUrl\" to be a string, " +
> +            pprint`got ${json.proxyAutoconfigUrl}`);

If you use backticks on :277 also you can avoid the quote escaping, which I think would be preferable.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8934315 [details]
Bug 1420577 - Improve failure messages for invalid proxy configurations.

https://reviewboard.mozilla.org/r/205236/#review210820

> If you use backticks on :277 also you can avoid the quote escaping, which I think would be preferable.

In a different patch you didn't want to have backticks because no variable substituation has been taken place. Now it's vice versa. I will make this change given that I also find it looks better.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1420577 - Add template literals to eslint max-len ignore list.

Review commit: https://reviewboard.mozilla.org/r/205340/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/205340/

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8934315 [details]
Bug 1420577 - Improve failure messages for invalid proxy configurations.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/205236/diff/1-2/

Comment 6

[Andreas Tolfsen ❲:ato❳]

(In reply to Henrik Skupin (:whimboo) from comment #3)

> > If you use backticks on :277 also you can avoid the quote
> > escaping, which I think would be preferable.
> 
> In a different patch you didn't want to have backticks because
> no variable substituation has been taken place. Now it's vice
> versa. I will make this change given that I also find it looks
> better.

I think maybe you misunderstand.  Template strings are there to
make code more beautiful but according to JS developers I have
talked to they are not JITed and can therefore be slower to use than
regular quoted strings.  From that I conclude that they shouldn’t be
over-used when there is no need, but if using them makes code more
readable, I know which option I would pick.

Comment 7

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8934419 [details]
Bug 1420577 - Add template literals to eslint max-len ignore list.

https://reviewboard.mozilla.org/r/205340/#review210984

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Andreas Tolfsen ‹:ato› from comment #6)
> I think maybe you misunderstand.  Template strings are there to
> make code more beautiful but according to JS developers I have
> talked to they are not JITed and can therefore be slower to use than
> regular quoted strings.  From that I conclude that they shouldn’t be

That one I didn't know. Thanks for the details. Maybe the slowness doesn't apply if no variable substitution takes place in the template string? Do you know about that?

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/732efd9df3ec
Add template literals to eslint max-len ignore list. r=ato
https://hg.mozilla.org/integration/autoland/rev/2101004c5245
Improve failure messages for invalid proxy configurations. r=ato

Comment 10

[Andreas Tolfsen ❲:ato❳]

(In reply to Henrik Skupin (:whimboo) from comment #8)

> (In reply to Andreas Tolfsen ‹:ato› from comment #6)
> 
> > I think maybe you misunderstand.  Template strings are there to
> > make code more beautiful but according to JS developers I have
> > talked to they are not JITed and can therefore be slower to use
> > than regular quoted strings.  From that I conclude that they
> > shouldn’t be
> 
> That one I didn't know. Thanks for the details. Maybe the slowness
> doesn't apply if no variable substitution takes place in the
> template string? Do you know about that?

I think that is a good guess, but I don’t have first hand proof of
that.

In any case, I was just making a suggestion to avoid having quoted
strings inside strings.

Comment 11

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/732efd9df3ec
https://hg.mozilla.org/mozilla-central/rev/2101004c5245

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Those minor wording changes for failures in capabilities for proxy settings will greatly improve the handling in geckodriver. Please uplift those test harness changes to beta. Thanks.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/965c4569f8c4
https://hg.mozilla.org/releases/mozilla-beta/rev/e76ff14f1538