[🚀 Feature]: Selenium Manager change outputs

[titusfortner]

Feature and motivation

@nvborisenko proposes:

if success - exit code should be 0, and the latest line in output should be exact the path to the binary".

@symonk proposes:

never output any logging level to stdout or stderr, just write errors to stderr and non errors to stdout, that way it's easier to pipe to other tools etc and not require every consumer who cares about the path output have to do parsing to get what they care about

My biggest take is that we shouldn't be putting error messages in stdout. I don't like having to remove INFO\t from results, especially since it is inconsistent with behavior of --version.

Maybe a compromise is to log the level when --debug is turned on, and leave it off otherwise?

Usage example

Current behavior

This is stdout with error code 0

$ ~/selenium-manager --browser firefox
INFO	/Users/titusfortner/.cache/selenium/geckodriver/mac64/0.32.0/geckodriver

This is also stdout with error code 0

$ ~/selenium-manager --version
selenium-manager 1.0.0-M2

This is stdout with error code > 0

$ ~/selenium-manager --browser firefoxes
Error: "Invalid browser/driver name"

This is stderr with error code > 0

$ ~/selenium-manager --what
error: Found argument '--what' which wasn't expected, or isn't valid in this context

  If you tried to supply '--what' as a value rather than a flag, use '-- --what'

Usage: selenium-manager [OPTIONS]

For more information try '--help'

stderr with error code > 0

$ ~/selenium-manager --browser
error: The argument '--browser <BROWSER>' requires a value but none was supplied

For more information try '--help'

[bonigarcia]

In my opinion, the log level (INFO, WARN. etc.) help to understand the log info, also when selenium-manager is executed directly from the shell (e.g. as an standalone app), so I would not remove this level. In the case of driver management, which is the big feature supported so far, the driver path is contained in a INFO log (which is unique, i.e., there is no more INFO logs). But even in that case, there could be WARN logs (see #11373) that contain warning messages that could be processed in the bindings to warn the user about some undesired behaviour (not and error, but something that can be potentially improved) to the final user.

[AutomatedTester]

Could we make the output computer consumable? E.g. Just print the messages within a JSON blob?

[titusfortner]

I was going to say that JSON seems like overkill, but then I got to thinking about how the bindings should convey potential additional information to the user.

Real world situation right now: Chrome dev channel doesn't have a compatible chromedriver released yet.
I think the most recent driver should be returned, but ideally the user should be notified that the versions aren't matched

The solution we have right now doesn't allow for anything that isn't either an error or the desired value. Maybe JSON is the right answer?

[titusfortner]

Out of curiosity, do we have examples of any other command line executable that adds log-like information to its sdout by default?

[AutomatedTester]

At Mozilla we had output that was machine readable. I only suggested JSON as it's simpler that writing our own consumer in each language. You can see an example of the output at Mozilla https://firefoxci.taskcluster-artifacts.net/H6oXKqcQTGy9xwcVmyFh0g/0/public/logs/live_backing.log

[titusfortner]

I agree, minimizing logic duplication across languages is the goal. So stdout would be JSON blob & stderr would be string with the issue?

The main problem with this, of course, is that it wouldn't look great to a user working with it on the command line directly. Were the tools at Mozilla only consumed by machine? Should there be a flag of some kind to toggle the kind of output?

[titusfortner]

Another advantage of JSON is that when we eventually add downloading the browser, we're going to want to output the location of the browser as well... JSON would be a good way to provide data about both driver & browser.

[bonigarcia]

We can implement a new flag in Selenium Manager (e.g., --json-output) to produce the JSON blob containing the output. By default, Selenium Manager displays the output using a more human-friendly (or at least developer-friendly) using different log levels (INFO, WARN, ...), like it is doing now. The bindings will invoke Selenium Manager with --json-output and parse the results.

To implement this, we need to define a format for this JSON. Maybe something like this?

{
  "warnings": [
    "This is a warning trace",
    "This is another warning trace"
  ],
  "debug": [
    "This is a debug trace",
    "This is another debug trace"
  ],
  "result" : "/home/boni/.cache/selenium/chromedriver/linux64/106.0.5249.61/chromedriver"
}

... or alternatively:

{
  "warnings": [
    {
      "timestamp": 1670343174,
      "message" : "This is a warning trace"
    },
    {
      "timestamp": 1670343483,
      "message" : "This is another warning trace"
    }
  ],
  "debug": [
    {
      "timestamp": 167034423,
      "message" : "This is a debug trace"
    },
    {
      "timestamp": 167034900,
      "message" : "This is another debug trace"
    },
  ],
  "result" : "/home/boni/.cache/selenium/chromedriver/linux64/106.0.5249.61/chromedriver"
}

Also, we need to decide what to do when unexpected errors (panic in Rust) happen. In that case (code results > 0), the JSON blob is not displayed, and the bindings get both sdtout and sdterr and use this info to throw an exception to the user. Right?

[bonigarcia]

Another attempt including output code:

{
  "warnings": [
    {
      "timestamp": 1670343174,
      "message" : "This is a warning trace"
    }
  ],
  "debug": [
    {
      "timestamp": 167034423,
      "message" : "This is a debug trace"
    }
  ],
  "result" : {
    "code" : 0,
    "message" : "/home/boni/.cache/selenium/chromedriver/linux64/106.0.5249.61/chromedriver"
  }
}

{
  "warnings": [
    {
      "timestamp": 1670343174,
      "message" : "This is a warning trace"
    }
  ],
  "debug": [
    {
      "timestamp": 167034423,
      "message" : "This is a debug trace"
    }
  ],
  "result" : {
    "code" : 65,
    "message" : "Wrong driver version"
  }
}

[AutomatedTester]

I would do something like

{
     "output": [
         {"type": "debug" 
            "timestamp": 167034423,
           "message" : "This is a debug trace"
         }
     ]
}

or just as each line needs to be output just do

 {"type": "debug" 
            "timestamp": 167034423,
           "message" : "This is a debug trace"
         }

so that as each line is output it can be consumed straight away.

[titusfortner]

There is no each line for consumption, really. Selenium sends the command and gets back a bunch of output.

Something like this is what would be easiest for bindings to parse:

{
code: 1,
error: "Bad things",
log: ["INFO\t1", "DEBUG\t2"]
warning: ["warning 1", "warning 2"]
output: {driver: "/path/to/driver",
         browser: "/path/to/browser",
         version: "v1.0"}
}

bindings pseudo code would be:

result = execute(selenium-manager, args)
if result.code > 0
  log result.log
  throw result.error

if debug-mode
  log result.log

log result.warning
return result.output 

[diemol]

Not 100% with @titusfortner on the contents, but with the format. Seems to me that a JSON with just one level is enough, why would we need to nest messages or an array of messages?

[bonigarcia]

Here's another attempt at the JSON format, which is very close to what happened on the Rust side:

For an ok execution (when resolving a driver correctly):

{
  "logs": [
    {
      "level": "WARN",
      "timestamp": 1670343174,
      "message" : "This is a warning trace"
    },
    {
      "level": "DEBUG",
      "timestamp": 167034423,
      "message" : "This is a debug trace"
    }
  ],
  "result" : {
    "code" : 0,
    "output" : "/home/boni/.cache/selenium/chromedriver/linux64/106.0.5249.61/chromedriver"
  }
}

For an error execution (when resolving a driver incorrectly):

{
  "logs": [
    {
      "level": "WARN",
      "timestamp": 1670343174,
      "message" : "This is a warning trace"
    },
    {
      "level": "DEBUG",
      "timestamp": 167034423,
      "message" : "This is a debug trace"
    }
  ],
  "result" : {
    "code" : 65,
    "output" : "Wrong driver version"
  }
}

In future operations, if needed, we can change the format of the output field to be richer, e.g.:

  "result" : {
    "code" : 0,
    "output" : {
      "key1": "value1",
      "field2" : "value2"
    }
  }

But for the current feature (i.e., driver management), that format is enough, IMO.

[titusfortner]

Ok, this makes sense. We're going to need to iterate over logs regardless, I think, so this is as good as can be expected.

One thing to keep in mind is that you can't change the format of the output and maintain backwards compatibility. As long as we're calling it a beta and not releasing it independently, we can do whatever we want. But we need to figure out how we want to handle changes.

[bonigarcia]

I've been reviewing this. Regarding the latter cases, I mean these:

This is stderr with error code > 0

$ ~/selenium-manager --what
error: Found argument '--what' which wasn't expected, or isn't valid in this context

  If you tried to supply '--what' as a value rather than a flag, use '-- --what'

Usage: selenium-manager [OPTIONS]

For more information try '--help'

stderr with error code > 0

$ ~/selenium-manager --browser
error: The argument '--browser <BROWSER>' requires a value but none was supplied

For more information try '--help'

This output is automatically created by the crate that Selenium Manager uses for parsing the CLI arguments, i.e., clap. I've been reviewing if this behavior can be tuned, but I think it is not possible, or at least, in a simple way. But in any case, these cases are not very relevant IMO since the bindings will not invoke Selenium Manager in that way. Therefore, I propose the following procedure:

1. I implement the --json-output argument on the Rust side using the previous format.
2. Selenium Manager is invoked always using that argument in all bindings.
3. The bindings try to parse the resulting output as JSON (captured in the stdout).
4. If result.code is 0, it means the operation was successful, and in that case, result.output will contain the result, which is the driver path in the case of driver management.
5. If result.code is different than 0, it means the operation was not successful, and in that case, result.output will contain a descriptive message about the error.
6. Independently of the result.code, the WARN logs should be displayed as warnings to the user since they can contain relevant information (e.g., when the Selenium Manager processes the PATH and it finds an outdated driver).
7. If the output cannot be parsed to JSON, an uncontrolled error has likely happened in Selenium Manager (like a Rust panic). In that case, both sdtout and stderr are captured by the bindings, using that message to raise an exception to the user.

What do you think?

[symonk]

on point 1 it might be better to have an --output <type> style flag rather than a hard coded flag for 1 individual type (if you think theres any benefit in that or others in future). anything point 6 warning related should be going to stderr only (imo) and not stdout. It should be a safe assumption that exiting 0 will include a path 100% of the time and keep in mind as a user of that tool I might want to | that output to something else for whatever purpose. I'm kind of on the fence about including all the log levels in the output at all but I haven't fully followed all the discussions around it

[p0deje]

I agree with @symonk that we if we intend to build the tool that can be used from command line, let's attempt to do it in a more UNIX way. I can imagine people can use it to copy binaries, append them to the PATH, etc so it would be nice if by default the tool:

1. prints a full path to the driver to STDOUT
2. prints any errors to STDERR
3. exits with 0 on success
4. exits with non-zero on issue

At the moment the following is not possible without extra processing of STDOUT to strip away INFO prefix.

# use selenium-manager to download driver and make it available globally
driver=$(bin/macos/selenium-manager --driver chromedriver)
export PATH="$(dirname driver):$PATH"

# use selenium-manager to download driver and symlink it to the local directory
bin/macos/selenium-manager --driver chromedriver | xargs -I{} ln -s {} ./chromedriver

If we follow these simple rules I suppose it will be the easiest way to use the tool both in the bindings and also as a generic UNIX tool.

[bonigarcia]

Maybe something in the middle could be as follows:

• --output json: JSON format proposed previously.
• --output shell: Approach proposed by @p0deje.

[titusfortner]

As an update, this was discussed at the Selenium TLC Meeting — https://www.selenium.dev/meetings/2022/tlc-12-08/#proposalsdecisions

We agreed that the output flags are the right approach.

[bonigarcia]

Complete with #11531.

[diemol]

Reopening so we can track bindings implementation

[diemol]

Python implemented through 00a2624

[diemol]

With .NET done, we can close this.

[github-actions]

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.