Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Description • 7 years ago

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.

	Andreas Tolfsen ❲:ato❳
	Comment 2 • 7 years ago
mozreview-review

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.

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 3 • 7 years ago
mozreview-review-reply

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.

	Andreas Tolfsen ❲:ato❳
	Comment 6 • 7 years ago

(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.

	Andreas Tolfsen ❲:ato❳
	Comment 7 • 7 years ago
mozreview-review

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

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

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 8 • 7 years ago

(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?

	Pulsebot
	Comment 9 • 7 years ago

Pushed by hskupin@mozilla.com:
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

	Andreas Tolfsen ❲:ato❳
	Comment 10 • 7 years ago

(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.

	Cosmin Sabou [:CosminS]
	Comment 11 • 7 years ago
bugherder

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

	Henrik Skupin [:whimboo][⌚️UTC+1] Assignee
	Comment 12 • 7 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 13 • 7 years ago
bugherder uplift

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