-
-
Notifications
You must be signed in to change notification settings - Fork 8.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[rust] Support different output types (logger, json, shell) in Selenium Manager (#11365) #11531
Conversation
The consumer of the You mentioned that This is the kind of logic the bindings need:
I also think logs should be formatted as a string (similar to how webdriver spec does stack traces instead of a nested array. So I'd prefer to see this structure:
or
Then if
|
I assume that was merely informative, as the bindings will get that information back. Right, @bonigarcia? |
@diemol Yes, that info is informative. I am not sure if the bindings are going to require that info. As @titusfortner suggested, I have just committed a second patch to this PR to flush these logs ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A minor comment from my side.
@titusfortner To simplify things in the bindings, I have just added another patch to include the result code and message. See some examples:
|
This PR contains a couple of tests to assess this feature. I think something similar to this should be done in the bindings (i.e., checking |
I get we shouldn't optimize for what we haven't implemented yet, but if we know we're going to be returning browser location at some point, then we know just having a single message line is insufficient. In logger mode it can just be another info line, and for shell most likely it's going to actually need to be two subsequent calls, once for browser and once for driver. In json mode, though we can use a different key name instead of a common message key. The logging focus still makes it hard to parse. The user doesn't care about timestamps or levels, they just want the result and to see any important caveats (warnings). We shouldn't need to write the same method in 5 different languages in order to parse an array of objects to pull out and reformat the info the user wants to see. How about by default we have an array of warning strings without the duplicated info string and without the timestamps?
|
I think the timestamp is important since Selenium Manager is used in the Grid as well. And eventually it could help to debug issues on tests executed in CI or similar. Which duplicated info string do you mean? |
Grid should be logging it with Java timestamps, and it's not like each microsecond step needs its own stamp here. |
Removing the timestamp in Rust is straightforward. Ignoring that info from the bindings is also straightforward, IMO. If there is a chance that that info can be helpful in some scenario (e.g., Grid or other third-party), my bet is to leave the JSON as it is now. It is a tiny detail, IMO. |
I think if we want Selenium Manager to be used in the future by third parties, we should have a structured log (which includes a time stamp), and discarding information is easier than adding it when a wide audience is using the tool. |
Selenium has a history of trying to support lots of theoretical use cases instead instead of making the primary known case straightforward. I guess I'm not going to be the one implementing it, so I'll leave the parsing logic to you. |
Sounds good, so we can leave it there and a client using Selenium Manager can ignore the values if desired. |
Description
This PR implements the flag
--output
for Selenium Manager. The accepted values for this flag are:LOGGER
: Usual log traces (INFO
,WARN
, etc.) used in the CLI. Default value.JSON
: Custom JSON notation (see below).SHELL
: Minimal Unix-like output (i.e., only full path when the driver is resolved or an error message if not).To simplify things, the implemented JSON notation in this PR is simpler than proposed in #11365. The JSON basically contains the logs (level, timestamp, and message) displayed by Selenium Manager.
The bindings can invoke Selenium Manager using
--output json
and try parse its output. Then, the protocol to interpret the parsed JSON is as follows:0
: The last (and only)INFO
message contains the driver path (when in future releases Selenium Manager also manages browsers, this message can be the browser path).0
: The last (and only)ERROR
message contains the error message to throw an exception to the user.In both cases, the
WARN
messages can be displayed to the user, since they contain some warning to the users (e.g., when a fallback is used to resolve the driver). In principle, theDEBUG
andTRACE
logs can safely ignored in the bindings.Does it make sense?
An example of the JSON output is as follows (running Selenium Manager from the source code with Cargo):
Motivation and Context
Feature requested on #11365. Discussed at the Selenium TLC Meeting.
Types of changes
Checklist