Closed Bug 1526938 Opened 6 years ago Closed 6 years ago

Error descriptions are discarded and no error printed on flag parsing

Categories

(Testing :: geckodriver, enhancement, P1)

Version 3
enhancement

Tracking

(firefox67 fixed)

RESOLVED FIXED
mozilla67
Tracking Status
firefox67 --- fixed

People

(Reporter: ato, Assigned: ato)

References

Details

Attachments

(13 files)

(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details

We discard error descriptions when a problem arises during flag
parsing in geckodriver, typically reducing them to a vague description
about what flag was parsed e.g. "invalid host name". We should
pick up the description from the error and print that to the user.

Secondly, we fail to print anything to stderr when flag parsing
fails due to using the error!() macro from the log crate. When
parsing flags we have not yet enabled the logging subsystem, which
means it defaults to not printing anything. We cannot initialise
it sooner because we may risk printing log entries the user hasn’t
requested.

Assignee: nobody → ato
Priority: -- → P1
Blocks: 1525659

Making use of clap::Arg::default_value() removes duplicated magic
strings, by not having to use .unwrap_or("default value").

Depends on D19423

We don't need an enum for exit codes as we never have to match
on them. Converting them to consts also has the benefit that we
will not have to coerce them to i32.

i32 is the correct type to use here, since it is what std::process::exit()
takes and what the libc crate uses.

Depends on D19424

Instead of returning a tuple of (i32, String), causing us to lose
details and the cause of an error, this patch formalises the error
scenarios that may occur during command-line parsing and startup
of the WebDriver server.

Exit codes are encoded with each variant of FatalError, instead of
being specified in each case.

Type conversions are implemented for clap::Error (flag parsing)
and io::Error (webdriver::server).

Depends on D19426

We replaced --webdriver-port with --port to better match
EdgeDriver and chromedriver, but it was discovered in
https://github.com/mozilla/geckodriver/issues/154 that this broke
Selenium clients.

We decided on a two-spearhead approach by reintroducing --webdriver-port
as a temporary alias, whilst submitting fixes for the Selenium clients.
The fixes to Selenium landed over three years ago.

This patch removes the --webdirver-port alias from geckodriver.

Depends on D19427

Depends on D19428

Because we emit the flag parsing errors to the log, through error!(),
they are subject to whether the logging subsystem is enabled.
Because logging is disabled by default, no error information is
currently displayed to the user.

Since we cannot initialise logging implicitly due to the risk of
emitting log messages the user did not request, this patch changes
geckodriver to print the flag parsing errors to stderr

Depends on D19429

The help message is implicitly included in clap error descriptions, but
they are not for errors originating from this file. By introducing
a FatalError::help_included() function we make sure we print the
help message in all cases.

An alternative implementation, which perhaps would be more idiomatic,
would be to prepare the help message within the fmt::Display trait
implementation for FatalError, but I could find no way of passing
in a reference to clap::App without storing it in an atomic global const.

Depends on D19430

Flag parsing and application logic belong to one, very long,
spaghetti-like function. This patch introduces a distinction
between checking the sanity of the CLI input and what action to
take depending on that input.

Depends on D19431

For completeness, this makes sure the --help flag is included in
its own help message.

Depends on D19432

This will print something along the line of:

geckodriver: error: invalid --port: invalid digit found in string: asd

Or:

geckodriver: error: invalid --port: number too large to fit in target type: 123123123123

We also Include the error message from IpAddr::from_str() to the user.
This will make the error seen by the user look something like this:

geckodriver: error: invalid IP address syntax: 123123:4444

Depends on D19433

Minor post-patch linting to silence some clippy warnings.

Depends on D19434

Status: NEW → ASSIGNED
Pushed by atolfsen@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a9357f295d5f geckodriver: display same version on --help as on --version; r=whimboo https://hg.mozilla.org/integration/autoland/rev/e2a10e5049be geckodriver: make use of clap default values; r=whimboo https://hg.mozilla.org/integration/autoland/rev/f11ef5763129 geckodriver: convert ExitCode to consts; r=whimboo https://hg.mozilla.org/integration/autoland/rev/342fcc83b613 geckodriver: import logging::Level into the global scope; r=whimboo https://hg.mozilla.org/integration/autoland/rev/0fdfd128248c geckodriver: generalise cli errors; r=whimboo https://hg.mozilla.org/integration/autoland/rev/dfb9154473ca geckodriver: remove --webdriver-port alias; r=whimboo https://hg.mozilla.org/integration/autoland/rev/6ad3a665abdf geckodriver: organise args a bit more consistently; r=whimboo https://hg.mozilla.org/integration/autoland/rev/e37346a02373 geckodriver: fix missing flag parsing errors; r=whimboo https://hg.mozilla.org/integration/autoland/rev/16aac9625fde geckodriver: display help message on non-clap errors; r=whimboo https://hg.mozilla.org/integration/autoland/rev/b2768473a280 geckodriver: separate flag parsing and application logic; r=whimboo https://hg.mozilla.org/integration/autoland/rev/561a8cb2db9b geckodriver: ensure --help is listed in help message; r=whimboo https://hg.mozilla.org/integration/autoland/rev/a02142b7aeff geckodriver: provide better error messages for flags; r=whimboo https://hg.mozilla.org/integration/autoland/rev/9469362459f9 geckodriver: lint main.rs; r=whimboo
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: