-
-
Notifications
You must be signed in to change notification settings - Fork 8.5k
[dotnet] Enabling drivers to set log to console. #16097
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
base: trunk
Are you sure you want to change the base?
Changes from all commits
1faee93
f5989f6
2d9d8ba
1260f0e
befc3e3
8b6c25e
49701a6
2e27142
513ad67
3989e9f
54a1a19
7199b94
749637b
a3358e6
fb64aba
72d12cd
e8ce574
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,6 +26,7 @@ | |
using System.Net; | ||
using System.Net.Http; | ||
using System.Threading.Tasks; | ||
using OpenQA.Selenium.Internal.Logging; | ||
|
||
namespace OpenQA.Selenium; | ||
|
||
|
@@ -37,6 +38,8 @@ public abstract class DriverService : ICommandServer | |
private bool isDisposed; | ||
private Process? driverServiceProcess; | ||
|
||
private static readonly ILogger _logger = Log.GetLogger(typeof(DriverService)); | ||
|
||
/// <summary> | ||
/// Initializes a new instance of the <see cref="DriverService"/> class. | ||
/// </summary> | ||
|
@@ -151,6 +154,12 @@ public int ProcessId | |
/// </summary> | ||
public string? DriverServicePath { get; set; } | ||
|
||
/// <summary> | ||
/// Collects the driver log output and writes it to the console. Defaults to <see langword="true"/>. | ||
/// Internal variable to avoid using the stream when logs are sent to a file. | ||
/// </summary> | ||
protected bool WriteDriverLogToConsole { get; set; } = true; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am pretty sure we don't need this property, despite its name is not clear. Just don't introduce it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In the case of sending GeckoDriver logs to a file, if we don't have a flag, even if it is internal, the streams get consumed by the console and the file. This ends up in an error because the console consumes the stream, and nothing gets sent to the file. That is why we need it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now, when we are listening logs as sync, finally we can remove this unnecessary property? |
||
|
||
/// <summary> | ||
/// Gets the command-line arguments for the driver service. | ||
/// </summary> | ||
|
@@ -243,6 +252,12 @@ public void Start() | |
this.driverServiceProcess.StartInfo.UseShellExecute = false; | ||
this.driverServiceProcess.StartInfo.CreateNoWindow = this.HideCommandPromptWindow; | ||
|
||
this.driverServiceProcess.StartInfo.RedirectStandardOutput = true; | ||
this.driverServiceProcess.StartInfo.RedirectStandardError = true; | ||
|
||
this.driverServiceProcess.OutputDataReceived += (s, e) => this.OnDriverProcessDataReceived(s, e, isError: false); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please rename: |
||
this.driverServiceProcess.ErrorDataReceived += (s, e) => this.OnDriverProcessDataReceived(s, e, isError: true); | ||
|
||
DriverProcessStartingEventArgs eventArgs = new DriverProcessStartingEventArgs(this.driverServiceProcess.StartInfo); | ||
this.OnDriverProcessStarting(eventArgs); | ||
|
||
|
@@ -251,6 +266,9 @@ public void Start() | |
DriverProcessStartedEventArgs processStartedEventArgs = new DriverProcessStartedEventArgs(this.driverServiceProcess); | ||
this.OnDriverProcessStarted(processStartedEventArgs); | ||
|
||
this.driverServiceProcess.BeginOutputReadLine(); | ||
this.driverServiceProcess.BeginErrorReadLine(); | ||
|
||
if (!serviceAvailable) | ||
{ | ||
throw new WebDriverException($"Cannot start the driver service on {this.ServiceUrl}"); | ||
|
@@ -308,6 +326,27 @@ protected virtual void OnDriverProcessStarted(DriverProcessStartedEventArgs even | |
this.DriverProcessStarted?.Invoke(this, eventArgs); | ||
} | ||
|
||
/// <summary> | ||
/// Handles the output and error data received from the driver process. | ||
/// </summary> | ||
/// <param name="sender">The sender of the event.</param> | ||
/// <param name="args">The data received event arguments.</param> | ||
/// <param name="isError">A value indicating whether the data received is from the error stream.</param> | ||
protected virtual void OnDriverProcessDataReceived(object sender, DataReceivedEventArgs args, bool isError) | ||
{ | ||
if (string.IsNullOrEmpty(args.Data)) | ||
return; | ||
|
||
if (this.WriteDriverLogToConsole && !isError && _logger.IsEnabled(LogEventLevel.Info)) | ||
{ | ||
_logger.Info(args.Data); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We have never used But I think logs from driver service is something low-level, I propose always put them to |
||
} | ||
if (this.WriteDriverLogToConsole && isError && _logger.IsEnabled(LogEventLevel.Error)) | ||
{ | ||
_logger.Error(args.Data); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From one point of view it might be reasonable. From another point of view
|
||
} | ||
} | ||
|
||
/// <summary> | ||
/// Stops the DriverService. | ||
/// </summary> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,10 +19,11 @@ | |
|
||
using OpenQA.Selenium.Internal; | ||
using System; | ||
using System.Diagnostics; | ||
using System.Globalization; | ||
using System.IO; | ||
using System.Text; | ||
using System.Threading.Tasks; | ||
using OpenQA.Selenium.Internal.Logging; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Order namespaces (minor, up to you) |
||
|
||
namespace OpenQA.Selenium.Firefox; | ||
|
||
|
@@ -32,6 +33,7 @@ namespace OpenQA.Selenium.Firefox; | |
public sealed class FirefoxDriverService : DriverService | ||
{ | ||
private const string DefaultFirefoxDriverServiceFileName = "geckodriver"; | ||
private static readonly ILogger _logger = Log.GetLogger(typeof(FirefoxDriverService)); | ||
|
||
/// <summary> | ||
/// Process management fields for the log writer. | ||
|
@@ -219,56 +221,55 @@ protected override string CommandLineArguments | |
} | ||
|
||
/// <summary> | ||
/// Handles the event when the driver service process is starting. | ||
/// Called when the driver process is starting. This method sets up log file writing if a log path is specified. | ||
/// </summary> | ||
/// <param name="eventArgs">The event arguments containing information about the driver service process.</param> | ||
/// <remarks> | ||
/// This method initializes a log writer if a log path is specified and redirects output streams to capture logs. | ||
/// This method initializes a log writer if a log path is specified. | ||
/// </remarks> | ||
protected override void OnDriverProcessStarting(DriverProcessStartingEventArgs eventArgs) | ||
{ | ||
if (!string.IsNullOrEmpty(this.LogPath)) | ||
{ | ||
this.WriteDriverLogToConsole = false; | ||
string? directory = Path.GetDirectoryName(this.LogPath); | ||
if (!string.IsNullOrEmpty(directory) && !Directory.Exists(directory)) | ||
{ | ||
Directory.CreateDirectory(directory); | ||
} | ||
|
||
// Initialize the log writer | ||
logWriter = new StreamWriter(this.LogPath, append: true) { AutoFlush = true }; | ||
|
||
// Configure process to redirect output | ||
eventArgs.DriverServiceProcessStartInfo.RedirectStandardOutput = true; | ||
eventArgs.DriverServiceProcessStartInfo.RedirectStandardError = true; | ||
} | ||
|
||
base.OnDriverProcessStarting(eventArgs); | ||
} | ||
|
||
/// <summary> | ||
/// Handles the event when the driver process has started. | ||
/// Handles the output and error data received from the driver process and sends it to the log writer if available. | ||
/// </summary> | ||
/// <param name="eventArgs">The event arguments containing information about the started driver process.</param> | ||
/// <remarks> | ||
/// This method reads the output and error streams asynchronously and writes them to the log file if available. | ||
/// </remarks> | ||
protected override void OnDriverProcessStarted(DriverProcessStartedEventArgs eventArgs) | ||
/// <param name="sender">The sender of the event.</param> | ||
/// <param name="args">The data received event arguments.</param> | ||
/// <param name="isError">A value indicating whether the data received is from the error stream.</param> | ||
protected override void OnDriverProcessDataReceived(object sender, DataReceivedEventArgs args, bool isError) | ||
{ | ||
if (logWriter == null) return; | ||
if (eventArgs.StandardOutputStreamReader != null) | ||
if (string.IsNullOrEmpty(args.Data)) | ||
return; | ||
|
||
if (!string.IsNullOrEmpty(this.LogPath)) | ||
{ | ||
_ = Task.Run(() => ReadStreamAsync(eventArgs.StandardOutputStreamReader)); | ||
if (logWriter != null) | ||
{ | ||
logWriter.WriteLine(args.Data); | ||
logWriter.Flush(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just to improve performance. |
||
} | ||
} | ||
|
||
if (eventArgs.StandardErrorStreamReader != null) | ||
else | ||
{ | ||
_ = Task.Run(() => ReadStreamAsync(eventArgs.StandardErrorStreamReader)); | ||
base.OnDriverProcessDataReceived(sender, args, isError); | ||
} | ||
|
||
base.OnDriverProcessStarted(eventArgs); | ||
} | ||
|
||
|
||
/// <summary> | ||
/// Disposes of the resources used by the <see cref="FirefoxDriverService"/> instance. | ||
/// </summary> | ||
|
@@ -368,24 +369,4 @@ private static string FirefoxDriverServiceFileName() | |
|
||
return fileName; | ||
} | ||
|
||
private async Task ReadStreamAsync(StreamReader reader) | ||
{ | ||
try | ||
{ | ||
string? line; | ||
while ((line = await reader.ReadLineAsync()) != null) | ||
{ | ||
if (logWriter != null) | ||
{ | ||
logWriter.WriteLine($"{DateTime.Now:yyyy-MM-dd HH:mm:ss.fff} {line}"); | ||
} | ||
} | ||
} | ||
catch (Exception ex) | ||
{ | ||
// Log or handle the exception appropriately | ||
System.Diagnostics.Debug.WriteLine($"Error reading stream: {ex.Message}"); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,25 +17,52 @@ | |
// under the License. | ||
// </copyright> | ||
|
||
using System; | ||
using System.Collections.Generic; | ||
using NUnit.Framework; | ||
using System.IO; | ||
using System.Linq; | ||
using OpenQA.Selenium.Internal.Logging; | ||
|
||
namespace OpenQA.Selenium.Firefox; | ||
|
||
[TestFixture] | ||
public class FirefoxDriverServiceTest : DriverTestFixture | ||
public class FirefoxDriverServiceTest | ||
{ | ||
private TestLogHandler testLogHandler; | ||
|
||
private void ResetGlobalLog() | ||
{ | ||
Log.SetLevel(LogEventLevel.Info); | ||
Log.Handlers.Clear().Handlers.Add(new TextWriterHandler(Console.Error)); | ||
} | ||
|
||
[SetUp] | ||
public void SetUp() | ||
{ | ||
ResetGlobalLog(); | ||
|
||
testLogHandler = new TestLogHandler(); | ||
} | ||
|
||
[TearDown] | ||
public void TearDown() | ||
{ | ||
ResetGlobalLog(); | ||
} | ||
|
||
[Test] | ||
public void ShouldRedirectGeckoDriverLogsToFile() | ||
{ | ||
FirefoxOptions options = new FirefoxOptions(); | ||
string logPath = Path.GetTempFileName(); | ||
options.LogLevel = FirefoxDriverLogLevel.Trace; | ||
options.LogLevel = FirefoxDriverLogLevel.Info; | ||
|
||
FirefoxDriverService service = FirefoxDriverService.CreateDefaultService(); | ||
service.LogPath = logPath; | ||
|
||
IWebDriver driver2 = new FirefoxDriver(service, options); | ||
IWebDriver firefoxDriver = new FirefoxDriver(service, options); | ||
firefoxDriver.Quit(); | ||
|
||
try | ||
{ | ||
|
@@ -45,9 +72,44 @@ public void ShouldRedirectGeckoDriverLogsToFile() | |
} | ||
finally | ||
{ | ||
driver2.Quit(); | ||
File.Delete(logPath); | ||
} | ||
} | ||
|
||
[Test] | ||
public void ShouldRedirectGeckoDriverLogsToConsole() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Specific driver tests are ignored. It will be nice to reincarnate them here: #15536 Read and resolve this thread. |
||
{ | ||
Log.SetLevel(LogEventLevel.Info).Handlers.Add(testLogHandler); | ||
FirefoxOptions options = new FirefoxOptions(); | ||
options.LogLevel = FirefoxDriverLogLevel.Info; | ||
|
||
FirefoxDriverService service = FirefoxDriverService.CreateDefaultService(); | ||
|
||
IWebDriver firefoxDriver = new FirefoxDriver(service, options); | ||
|
||
try | ||
{ | ||
Assert.That(testLogHandler.Events, Has.Count.AtLeast(1)); | ||
Assert.That(testLogHandler.Events.Any(e => e.Message.Contains("geckodriver")), Is.True); | ||
} | ||
finally | ||
{ | ||
firefoxDriver.Quit(); | ||
} | ||
} | ||
} | ||
|
||
class TestLogHandler : ILogHandler | ||
{ | ||
public ILogHandler Clone() | ||
{ | ||
return this; | ||
} | ||
|
||
public void Handle(LogEvent logEvent) | ||
{ | ||
Events.Add(logEvent); | ||
} | ||
|
||
public IList<LogEvent> Events { get; internal set; } = new List<LogEvent>(); | ||
} |
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.
This might be to aggressive but since we don't want to do sync reads, we might delete it. Or deprecate first?
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 did a quick search in GitHub and other places for
DriverProcessStartedEventArgs
and did not see mentions.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 was surprised here too. My initial thoughts were "no, don't expose it, it is so internal".
I propose to... not sure what. Let's leave this thread unresolved.
May be:
Let's wait little bit.