Skip to content
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

[java] Include selenium-manager output in Java exception #11300

Merged
merged 1 commit into from
Nov 24, 2022

Conversation

bonigarcia
Copy link
Member

Description

This PR changes the exception message when some unexpected error happens executing Selenium Manager, including the command output instead of the command itself (which is already logged as a warning trace).

Motivation and Context

The Selenium Mangear output will allow to debug future problems (e.g. #11291) easily.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@titusfortner
Copy link
Member

With the currently released selenium-manager, if there is a problem, the output is always an empty string, so this change won't help.

Have the changes to the selenium manager error handling changed this?
Do we need to be looking at stdout or stderr or both?

@bonigarcia
Copy link
Member Author

When some unexpected error happens, the error message goes to stderr. So we need to capture both stdout and stderr. I updated this PR to capture also stderr, which was missing previously (thanks for pointing it out). So now, the variable output in the Java code is not empty when some error happens (this will be very useful for debugging in the future). I think this deserves to be done also in the rest of bindings.

@titusfortner
Copy link
Member

Can we still log the full command being executed, or does that show up in stderr?

@bonigarcia
Copy link
Member Author

We can include the command as well (I have just appended it in the current PR). Currently, the command itself does not add many value (since the selenium-manager binary is executed in a temporal folder which is later deleted). But if we decide to store the binary the the cache, it will help to reproducibility (i.e., the user will be able to invoke selenium-manager manually).

@titusfortner
Copy link
Member

Yeah, I was more thinking about going forward. Like, if you send arguments --driver-version 102 it will fail because it needs to be the full driver version, and we want to be able to have visibility into that.

@diemol diemol merged commit 84da5ed into trunk Nov 24, 2022
@diemol diemol deleted the se_mgr_output_err branch November 24, 2022 08:35
@sonarcloud
Copy link

sonarcloud bot commented Nov 24, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

0.0% 0.0% Coverage
0.0% 0.0% Duplication

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants