[dotnet] Stream Selenium Manager output to internal logging #17024
Conversation
PR TypeEnhancement, Tests Description
|
| Relevant files | |||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Enhancement |
|
PR Compliance Guide 🔍Below is a summary of compliance checks for this PR:
Compliance status legend🟢 - Fully Compliant🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label |
||||||||||||||||||||||||||||
PR Code Suggestions ✨Explore these optional code suggestions:
|
||||||||||||||||||||||
There was a problem hiding this comment.
Pull request overview
This PR modernizes how Selenium Manager is invoked from the .NET bindings, introduces a structured DiscoverBrowser API, and wires Selenium Manager’s streaming output into the internal logging system with timestamped log entries.
Changes:
- Replace the old
BinaryPathsstring-arguments API with a newDiscoverBrowserAPI returning a strongly typedDiscoveryResult, including options for browser/driver version, paths, proxy, and timeout. - Update
DriverFinderto useSeleniumManager.DiscoverBrowserand local constants for driver/browser path keys instead of manually constructing arguments. - Extend the internal logging stack (
ILogger,ILogContext,Logger) to support timestamped log emission, and use a regex-based parser to stream Selenium Manager logs into the .NET logging pipeline while enhancing process execution (timeouts, richer error messages).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| dotnet/src/webdriver/SeleniumManager.cs | Introduces the new DiscoverBrowser API, generic RunCommand with timeout and improved error logging, and regex-based streaming of Selenium Manager stderr logs into the internal logger, plus new DiscoveryOptions/DiscoveryResult types and source-generated JSON metadata. |
| dotnet/src/webdriver/DriverFinder.cs | Switches from manual argument construction to calling SeleniumManager.DiscoverBrowser, stores results in a local paths cache keyed by internal constants, and validates the resolved driver/browser paths. |
| dotnet/src/webdriver/Internal/Logging/Logger.cs | Adds a public LogMessage(DateTimeOffset, LogEventLevel, string) method to emit log events with explicit timestamps, and refactors existing log methods to use it. |
| dotnet/src/webdriver/Internal/Logging/LogContext.cs | Updates EmitMessage to accept a timestamp and use it when creating LogEvent instances, aligning the context with the new timestamp-aware logging. |
| dotnet/src/webdriver/Internal/Logging/ILogger.cs | Extends the ILogger interface with a timestamp-aware LogMessage method used by Selenium Manager’s log streaming. |
| dotnet/src/webdriver/Internal/Logging/ILogContext.cs | Updates EmitMessage’s contract to include a timestamp parameter so logging contexts can preserve event time from external sources. |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
dotnet/src/webdriver/SeleniumManager.cs:310
- The generic catch block here wraps all exceptions, including
WebDriverExceptioninstances thrown above (for non-zero exit codes or timeouts), which means the detailed error messages you build are only available inInnerExceptionand the outer message is the more generic "Error starting process". This is a regression in error clarity and will make diagnosing Selenium Manager failures harder. Consider leaving existingWebDriverExceptions unwrapped (for example by using acatch (Exception ex) when (ex is not WebDriverException)filter) so callers see the rich process/timeout message directly, while still wrapping unexpected exceptions.
if (process.ExitCode != 0)
{
var exceptionMessageBuilder = new StringBuilder($"Selenium Manager process exited abnormally with {process.ExitCode} code: {process.StartInfo.FileName} {arguments}");
if (!string.IsNullOrWhiteSpace(stdOutputBuilder.ToString()))
{
exceptionMessageBuilder.AppendLine();
exceptionMessageBuilder.AppendLine("--- Standard Output ---");
exceptionMessageBuilder.Append(stdOutputBuilder);
exceptionMessageBuilder.AppendLine("--- End Standard Output ---");
}
if (!string.IsNullOrWhiteSpace(errOutputBuilder.ToString()))
{
exceptionMessageBuilder.AppendLine();
exceptionMessageBuilder.AppendLine("--- Error Output ---");
exceptionMessageBuilder.Append(errOutputBuilder);
exceptionMessageBuilder.AppendLine("--- End Error Output ---");
}
throw new WebDriverException(exceptionMessageBuilder.ToString());
}
}
catch (Exception ex)
{
throw new WebDriverException($"Error starting process: {process.StartInfo.FileName} {arguments}", ex);
| void HandleStandardOutput(object sender, DataReceivedEventArgs e) | ||
| { | ||
| // Treat SM's logs always as Trace to avoid SM writing at Info level | ||
| if (_logger.IsEnabled(LogEventLevel.Trace)) | ||
| stdOutputBuilder.AppendLine(e.Data); | ||
| } | ||
|
|
||
| void HandleErrorOutput(object sender, DataReceivedEventArgs e) | ||
| { | ||
| if (e.Data is not null) | ||
| { | ||
| foreach (var entry in jsonResponse.Logs) | ||
| var match = LogMessageRegex.Match(e.Data); | ||
|
|
||
| if (match.Success) | ||
| { | ||
| _logger.Trace($"{entry.Level} {entry.Message}"); | ||
| var dateTime = DateTimeOffset.Parse(match.Groups[1].Value); | ||
| var logLevel = match.Groups[2].Value; | ||
| var message = match.Groups[3].Value; | ||
|
|
||
| switch (logLevel) | ||
| { | ||
| case "INFO": | ||
| _logger.LogMessage(dateTime, LogEventLevel.Info, message); | ||
| break; | ||
| case "WARN": | ||
| _logger.LogMessage(dateTime, LogEventLevel.Warn, message); | ||
| break; | ||
| case "ERROR": | ||
| _logger.LogMessage(dateTime, LogEventLevel.Error, message); | ||
| break; | ||
| case "DEBUG": | ||
| _logger.LogMessage(dateTime, LogEventLevel.Debug, message); | ||
| break; | ||
| case "TRACE": | ||
| default: | ||
| _logger.LogMessage(dateTime, LogEventLevel.Trace, message); | ||
| break; | ||
| } | ||
| } | ||
| else | ||
| { | ||
| errOutputBuilder.AppendLine(e.Data); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The new streaming of Selenium Manager stderr into the internal logging system (parsing log lines and mapping to ILogger.LogMessage) and the timeout behavior in RunCommand are not covered by tests, even though the logging subsystem already has dedicated tests in dotnet/test/common/Internal/Logging/LogTest.cs. Given the importance of this behavior for diagnosing driver discovery issues, consider adding focused tests (e.g., around the log-line parsing and error/timeout handling logic) to verify that different log levels are mapped correctly and that failures/timeouts surface the expected messages.
3 participants
The changes modernize how browser and driver paths are discovered, enhance logging capabilities, and simplify the interface for driver discovery.
🔗 Related Issues
Contributes to #13989
💥 What does this PR do?
BinaryPathsmethod and argument-string construction with a newDiscoverBrowserAPI that takes structured options, builds arguments internally, and returns aDiscoveryResultobject. This makes the API clearer, more robust, and easier to extend.DriverFinderand now use the newSeleniumManager.DiscoverBrowsermethod for driver and browser path resolution.DriverFinderto use local constants for key names instead of referencing them fromSeleniumManager.ILogger,ILogContext) to support emitting log messages with explicit timestamps, and updated implementations accordingly.🔄 Types of changes