-
-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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] Include mixed output (INFO, WARN, DEBUG, etc. to stderr and minimal JSON to stdout) #13414
Conversation
876a8b3
to
14ed560
Compare
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.
I prefer to avoid a value like mixed
because it can be confusing. Why don't we leave the output as it is and log JSON to stderr? I cannot imagine someone needing stdout in one format and stderr in another.
If that looks strange on the console. Then let's have a flag for the output/result, and a a flag for the log output. What do you think?
I think we want to get rid of
|
I've done that way for different reasons. First, because writing in the And more important, creating a new output type (called Having said that, I can update the PR if you still consider a new |
I guess it makes sense to return the results by default in a json structure because it is easier to consume, and ideally the ones who use this binary need to do that. Now, my doubt with this PR, is how can we show a good error message in the client bindings when an error happens? |
If status code > 0, then the stdout needs to be the error message. |
That, for me, is a bit against nature. Maybe the "minimal JSON", as I said in the PR, should contain the field already available in the former
And if you prefer, I can remove the |
You're right, my suggestion is not standard; disregard. 😁 We should be able to parse ERROR messages from stderr and put them in the exception, still? |
Using the approach I proposed in my last message, the error message should be in the Then. Should I implemented the last approach? If so, remember that it is a breaking change and all bindings should be updated to parse the new JSON etc. |
let's keep "mixed" for this release so we have time to update the implementation in all the bindings. Whether we replace json output (Diego's suggestion) or remove the toggle all together (mine), we can do that in next release. And sure, let's add "error" to stdout json. I'm not sure it's necessary, but we can remove it later if we need to. |
Sounds good. |
14ed560
to
86b1f4f
Compare
Ok then. I have just committed another patch to this PR to include the error message (i.e., a field called
|
86b1f4f
to
a2c35ff
Compare
Update: I have changed the field
|
23f20ec
to
db158c8
Compare
Should we merge this after releasing 4.18 to have time to update the bindings? |
f5c9049
to
7cd5bf3
Compare
Since the changes are only for The one important piece that is missing is |
Sure, I'll make a PR about it soon. |
|
7cd5bf3
to
b14f2ce
Compare
CI Failure Feedback 🧐(Checks updated until commit f4d2030)
✨ CI feedback usage guide:The CI feedback tool (
In addition to being automatically triggered, the tool can also be invoked manually by commenting on a PR:
where Configuration options
See more information about the |
…nimal JSON to stdout) (SeleniumHQ#13414) * [rust] Include mixed output (INFO, WARN, DEBUG, etc. to stderr and minimal JSON to stdout) * [rust] Include field for error message in the minimal JSON * [rust] Change field message to error * [rust] Fix logger module --------- Co-authored-by: Diego Molina <[email protected]>
Description
This PR includes a new type of output for Selenium Manager -called
mixed
- for the--output
argument . This output makes SM to write the regular logs (INFO
,WARN
,DEBUG
, etc.) to stderr while they are generated, and a minimal JSON output (composed bydriver_path
andbrowser_path
) to thestdout
and the end of the SM execution. This feature has been tested on Windows and Linux:NOTE 1: This PR also stops writing the value of the driver path in
result.message
of the old JSON format. This feature has been supported to provide compatibility with the initial versions of SM, but I believe this is not required anymore. Example:NOTE 2: The value of
driver_path
is also used for the grid path. Examples:Motivation and Context
This feature has been requested in the selenium_manager Slack channel.
Fixes #13260
Types of changes
Checklist