search

MachinePublishers / jBrowserDriver

A programmable, embeddable web browser driver compatible with the Selenium WebDriver spec -- headless, WebKit-based, pure Java
Other
809 stars 143 forks source link

Make Settings.java take loglevel #190

Closed Jazzepi closed 7 years ago

Jazzepi commented 7 years ago

Added ability to set log level on the Settings object. Previously it was only being set to a default level, and there was no way to set it directly. Fixed up maven compilation under Java 8. Would break on bad javadocs without linter turned off. *Minor refactorings here and there.

Jazzepi commented 7 years ago

This may fix issue #175. To be honest, I don't really understand why logWire, logTrace, and logWarnings are a thing. Setting the logging level seems like a much more direct route to the same outcome.

hollingsworthd commented 7 years ago

Thanks for submitting this! With this change, if the logger name is changed after the level is set, I don't think the level will have an affect. Also ideally the level type would be a string or int so that users passing Capabilities from a non-Java language can set the level. I don't think Selenium APIs have a level and this is why logging is the way it is rather than a more typical Java way. It wasn't until recently in this project that logging was added using Java Util Logging so from a design perspective, it's an afterthought.

I'll merge this because it compiles and works, but before next release the Level API should either be removed or modified as per comments here.

In regard to #175, the root cause is something deeper. This change and the one I recommended in that issue I think are basically workarounds.

Jazzepi commented 7 years ago

With this change, if the logger name is changed after the level is set, I don't think the level will have an affect.

Totally true, but you've got the loggingLevel wrapped up in state with the logger itself. If you wanted to preserve it moving it outside of the logger and then "reapplying" it after the build is done would be a better solution.

Also ideally the level type would be a string or int so that users passing Capabilities from a non-Java language can set the level

I am super confused here! I know JBrowser is designed to work with many JVM languages, but how does the Setting file effect it's interoperability with other languages? Is it somehow exposed in the Selenium API?

hollingsworthd commented 7 years ago

preserve it moving it outside of the logger and then "reapplying" it after the build is done

Something like that.

On interoperability, I was thinking the API would need to change to support a non-Java way to set the level, but actually it wouldn't need to. Selenium has a Capabilities API, which is basically just a map of key/values. The JBrowserDriver constructor accepts a Capabilities object, and it basically just constructs a Settings object out of it. So in that step, it could convert an int/string level and the API wouldn't change.

So on second thought, this change is probably suitable to be released since the API won't be changed/deprecated later.