Error descriptions are discarded and no error printed on flag parsing
Categories
(Testing :: geckodriver, enhancement, P1)
Tracking
(firefox67 fixed)
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 | ||
Updated•6 years ago
|
Assignee | ||
Comment 1•6 years ago
|
||
Assignee | ||
Comment 2•6 years ago
|
||
Making use of clap::Arg::default_value() removes duplicated magic
strings, by not having to use .unwrap_or("default value").
Depends on D19423
Assignee | ||
Comment 3•6 years ago
|
||
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
Assignee | ||
Comment 4•6 years ago
|
||
Depends on D19425
Assignee | ||
Comment 5•6 years ago
|
||
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
Assignee | ||
Comment 6•6 years ago
|
||
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
Assignee | ||
Comment 7•6 years ago
|
||
Depends on D19428
Assignee | ||
Comment 8•6 years ago
|
||
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
Assignee | ||
Comment 9•6 years ago
|
||
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
Assignee | ||
Comment 10•6 years ago
|
||
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
Assignee | ||
Comment 11•6 years ago
|
||
For completeness, this makes sure the --help flag is included in
its own help message.
Depends on D19432
Assignee | ||
Comment 12•6 years ago
|
||
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
Assignee | ||
Comment 13•6 years ago
|
||
Minor post-patch linting to silence some clippy warnings.
Depends on D19434
Assignee | ||
Updated•6 years ago
|
Comment 14•6 years ago
|
||
Comment 15•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a9357f295d5f
https://hg.mozilla.org/mozilla-central/rev/e2a10e5049be
https://hg.mozilla.org/mozilla-central/rev/f11ef5763129
https://hg.mozilla.org/mozilla-central/rev/342fcc83b613
https://hg.mozilla.org/mozilla-central/rev/0fdfd128248c
https://hg.mozilla.org/mozilla-central/rev/dfb9154473ca
https://hg.mozilla.org/mozilla-central/rev/6ad3a665abdf
https://hg.mozilla.org/mozilla-central/rev/e37346a02373
https://hg.mozilla.org/mozilla-central/rev/16aac9625fde
https://hg.mozilla.org/mozilla-central/rev/b2768473a280
https://hg.mozilla.org/mozilla-central/rev/561a8cb2db9b
https://hg.mozilla.org/mozilla-central/rev/a02142b7aeff
https://hg.mozilla.org/mozilla-central/rev/9469362459f9
Description
•