Set log verbosity from capability #217

Merged
merged 4 commits into from Sep 7, 2016

Projects

None yet

2 participants

@andreastt
Member
andreastt commented Sep 6, 2016 edited

This change is Reviewable

@andreastt
Member
@jgraham
Collaborator
jgraham commented Sep 6, 2016

Reviewed 3 of 6 files at r1.
Review status: 3 of 6 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


src/marionette.rs, line 451 [r1] (raw file):

                    let s = try!(json.as_string()
                        .ok_or(WebDriverError::new(ErrorStatus::InvalidArgument, "Log level is not a string")));
                    let l = try!(LogLevel::from_str(s).ok()

Don't use l as a variable name. Just

Some(try!(LogLevel::from_str(s)
                    .ok()
                   .ok_or(WebDriverError::new(ErrorStatus::InvalidArgument, "Log level is unknown"))))

would be fine.


src/marionette.rs, line 460 [r1] (raw file):

            Ok(LogOptions {
                level: level,
                file: None,

It's not really clear why this is always None.


src/marionette.rs, line 507 [r1] (raw file):

}

// env_logger may only be initialised once. This function should not be

This seems troublesome because we certianly want logging before the http connection is established for diagnosing problems in geckodriver itself. Also it doesn't work if we get a second session after the first has quit.


Comments from Reviewable

@jgraham
Collaborator
jgraham commented Sep 6, 2016

Reviewed 3 of 6 files at r1.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


Comments from Reviewable

@jgraham jgraham added this to the 0.11 milestone Sep 7, 2016
andreastt added some commits Aug 22, 2016
@andreastt andreastt fix shorthand verbosity flags cc334f3
@andreastt andreastt allow firefoxOptions.log.level capability to control logging
The firefoxOptions.log.level capability may optionally be passed to the
New Session command, which will initialise the env_logger and override
the verbosity level requested from command-line flags or the RUST_LOG
environment variable.

When the flags are used these will from now on also enable the env_logger.
8706df6
@andreastt andreastt remove default implementation for MarionetteSettings 2b35215
@andreastt andreastt raise default log level to info
b6115e6
@andreastt
Member

Review status: 0 of 6 files reviewed at latest revision, 3 unresolved discussions.


src/marionette.rs, line 451 [r1] (raw file):

Previously, jgraham wrote…

Don't use l as a variable name. Just

Some(try!(LogLevel::from_str(s)
                    .ok()
                   .ok_or(WebDriverError::new(ErrorStatus::InvalidArgument, "Log level is unknown"))))

would be fine.

Done.

src/marionette.rs, line 460 [r1] (raw file):

Previously, jgraham wrote…

It's not really clear why this is always None.

Done.

src/marionette.rs, line 507 [r1] (raw file):

Previously, jgraham wrote…

This seems troublesome because we certianly want logging before the http connection is established for diagnosing problems in geckodriver itself. Also it doesn't work if we get a second session after the first has quit.

Done.

Comments from Reviewable

@jgraham
Collaborator
jgraham commented Sep 7, 2016

Reviewed 5 of 6 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@jgraham jgraham merged commit 2277355 into mozilla:master Sep 7, 2016

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
code-review/reviewable 6 files reviewed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment