1396819 - Update wording for expected values in validity checks

Status: RESOLVED FIXED

(Testing :: geckodriver, enhancement, P3)

Description

[Andreas Tolfsen ❲:ato❳]

https://github.com/mozilla/webdriver-rust/issues/122

> As requested by @andreastt on #121 (comment):
> 
>> Generally we should avoid passive language in error messages,
>> and I think we could afford to be a tad more specific
>> regarding which key this applies to.
> 
> I suggest: `Expected noProxy to be an array, got: {}.`
> 
> We should check for which keys this wording can be used.

Comment 1

[Andreas Tolfsen ❲:ato❳]

Attachment: Bug 1396819 - Improve error messages from capabilities matching.

This patch somewhat marginally improves error messages returned
during capabilities negotiation.

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

Comment 2

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

Comment on attachment 8950943 [details]
Bug 1396819 - Improve error messages from capabilities matching.

https://reviewboard.mozilla.org/r/220194/#review226752

Just nits which need to be fixed. Thanks!

::: testing/webdriver/src/capabilities.rs:90
(Diff revision 1)
>  
>          for (key, value) in capabilities.iter() {
>              match &**key {
>                  "acceptInsecureCerts" => if !value.is_boolean() {
>                          return Err(WebDriverError::new(ErrorStatus::InvalidArgument,
> -                                                       "acceptInsecureCerts is not a boolean"))
> +                                                       format!("acceptInsecureCerts is not boolean boolean: {}", value)))

boolean exist twice here.

::: testing/webdriver/src/capabilities.rs:184
(Diff revision 1)
> -                "socksProxy" => try!(SpecNewSessionParameters::validate_host(value)),
> +                "sslProxy" => SpecNewSessionParameters::validate_host(value, "sslProxy")?,
> +                "socksProxy" => SpecNewSessionParameters::validate_host(value, "socksProxy")?,
>                  "socksVersion" => if !value.is_number() {
> -                    return Err(WebDriverError::new(ErrorStatus::InvalidArgument,
> -                                                   "socksVersion is not a number"))
> +                    return Err(WebDriverError::new(
> +                        ErrorStatus::InvalidArgument,
> +                        "socksVersion is not a number"

Please format the value as specified.

::: testing/webdriver/src/capabilities.rs:221
(Diff revision 1)
>          Ok(())
>      }
>  
>      /// Validate whether a named capability is JSON value is a string containing a host
>      /// and possible port
> -    fn validate_host(value: &Json) -> WebDriverResult<()> {
> +    fn validate_host(value: &Json, entry: &str) -> WebDriverResult<()> {

Maybe reversing the order?

::: testing/webdriver/src/capabilities.rs:227
(Diff revision 1)
>          match value.as_string() {
>              Some(host) => {
>                  if host.contains("://") {
>                      return Err(WebDriverError::new(
>                          ErrorStatus::InvalidArgument,
> -                        format!("{} contains a scheme", host)));
> +                        format!("{} may not contain a scheme: {}", entry, host)));

Shouldn't this be `should not contain`?

::: testing/webdriver/src/capabilities.rs:286
(Diff revision 1)
>      }
>  
>      fn validate_unhandled_prompt_behaviour(value: &Json) -> WebDriverResult<()> {
>          let behaviour = try_opt!(value.as_string(),
>                                   ErrorStatus::InvalidArgument,
> -                                 "unhandledPromptBehavior capability is not a string");
> +                                 "unhandledPromptBehavior is not a string");

Please format the value.

::: testing/webdriver/src/capabilities.rs:304
(Diff revision 1)
>  
>  impl Parameters for SpecNewSessionParameters {
>      fn from_json(body: &Json) -> WebDriverResult<SpecNewSessionParameters> {
>          let data = try_opt!(body.as_object(),
>                              ErrorStatus::UnknownError,
> -                            "Message body is not an object");
> +                            "Malformed capabilities, message body is not an object");

Would it make sense to format the malformed data?

Comment 3

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8950943 [details]
Bug 1396819 - Improve error messages from capabilities matching.

https://reviewboard.mozilla.org/r/220194/#review226752

> Would it make sense to format the malformed data?

Sure.  All good ideas I’ve implemented.

Comment 4

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8950943 [details]
Bug 1396819 - Improve error messages from capabilities matching.

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

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1f94fa48c7dc
Improve error messages from capabilities matching. r=whimboo

Comment 6

[Raul Gurzau (:RaulG)]

Backed out changeset 1f94fa48c7dc (bug 1396819) for build bustage

Backout link: https://hg.mozilla.org/integration/autoland/rev/ca5ab7d0c716eed612ef41fb433806de231dce4e

Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=1f94fa48c7dc1452be45418ef956caf6de054c29

Log link: https://treeherder.mozilla.org/logviewer.html#?job_id=162996234&repo=autoland&lineNumber=26989

[task 2018-02-19T12:09:24.965Z] 12:09:24     INFO -  error: aborting due to previous error
[task 2018-02-19T12:09:24.966Z] 12:09:24     INFO -  error: Could not compile `webdriver`.
[task 2018-02-19T12:09:24.966Z] 12:09:24     INFO -  Caused by:
[task 2018-02-19T12:09:24.966Z] 12:09:24     INFO -    process didn't exit successfully: `/builds/worker/workspace/build/src/sccache2/sccache /builds/worker/workspace/build/src/rustc/bin/rustc --crate-name webdriver /builds/worker/workspace/build/src/testing/webdriver/src/lib.rs --crate-type lib --emit=dep-info,link -C opt-level=3 -C metadata=91dbbaf1a25660af -C extra-filename=-91dbbaf1a25660af --out-dir /builds/worker/workspace/build/src/obj-firefox/testing/geckodriver/./i686-unknown-linux-gnu/release/deps --target i686-unknown-linux-gnu -C linker=/builds/worker/workspace/build/src/build/cargo-linker -L dependency=/builds/worker/workspace/build/src/obj-firefox/testing/geckodriver/./i686-unknown-linux-gnu/release/deps -L dependency=/builds/worker/workspace/build/src/obj-firefox/testing/geckodriver/./release/deps --extern regex=/builds/worker/workspace/build/src/obj-firefox/testing/geckodriver/./i686-unknown-linux-gnu/release/deps/libregex-5f07b590822dbae6.rlib --extern rustc_serialize=/builds/worker/workspace/build/src/obj-firefox/testing/geckodriver/./i686-unknown-linux-gnu/release/deps/librustc_serialize-9a2faa3dbbfd72a1.rlib --extern time=/builds/worker/workspace/build/src/obj-firefox/testing/geckodriver/./i686-unknown-linux-gnu/release/deps/libtime-9ec2af0ecc3ff91f.rlib --extern hyper=/builds/worker/workspace/build/src/obj-firefox/testing/geckodriver/./i686-unknown-linux-gnu/release/deps/libhyper-371624cfe9cb6b85.rlib --extern url=/builds/worker/workspace/build/src/obj-firefox/testing/geckodriver/./i686-unknown-linux-gnu/release/deps/liburl-eaeb96112a6b6cfc.rlib --extern log=/builds/worker/workspace/build/src/obj-firefox/testing/geckodriver/./i686-unknown-linux-gnu/release/deps/liblog-7c2df9f1066cc662.rlib --extern cookie=/builds/worker/workspace/build/src/obj-firefox/testing/geckodriver/./i686-unknown-linux-gnu/release/deps/libcookie-2efebd3de4550abb.rlib -C opt-level=2 -C debuginfo=2` (exit code: 101)
[task 2018-02-19T12:09:24.967Z] 12:09:24     INFO -  warning: build failed, waiting for other jobs to finish...
[task 2018-02-19T12:09:24.968Z] 12:09:24     INFO -  error: build failed
[task 2018-02-19T12:09:24.968Z] 12:09:24     INFO -  /builds/worker/workspace/build/src/config/rules.mk:1016: recipe for target 'force-cargo-program-build' failed
[task 2018-02-19T12:09:24.968Z] 12:09:24     INFO -  make[4]: *** [force-cargo-program-build] Error 101

Comment 7

[Andreas Tolfsen ❲:ato❳]

Sloppy of me to push without compiling after the final change.

Comment 8

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8950943 [details]
Bug 1396819 - Improve error messages from capabilities matching.

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/387d87eb05f8
Improve error messages from capabilities matching. r=whimboo

Comment 10

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/387d87eb05f8