From e68af4a4f054ef872dfaf92504ad93268f49ec3e Mon Sep 17 00:00:00 2001 From: Craig Fowler Date: Sat, 21 Mar 2026 12:34:24 +0000 Subject: [PATCH] Resolve #57 - Remove early exit from parsing function --- .../WebDriverFactoryIntegrationTests.cs | 17 +++++ .../WebDriverFromThirdPartyFactoryTests.cs | 18 +++++ ...ings.WebDriverFactoryIntegrationTests.json | 6 ++ .../WebDriverConfigurationItemParser.cs | 67 ++++++++++--------- .../Factories/WebDriverCreationOptions.cs | 22 +++++- 5 files changed, 94 insertions(+), 36 deletions(-) diff --git a/CSF.Extensions.WebDriver.Tests/Factories/WebDriverFactoryIntegrationTests.cs b/CSF.Extensions.WebDriver.Tests/Factories/WebDriverFactoryIntegrationTests.cs index 78a4402..4c1c6a5 100644 --- a/CSF.Extensions.WebDriver.Tests/Factories/WebDriverFactoryIntegrationTests.cs +++ b/CSF.Extensions.WebDriver.Tests/Factories/WebDriverFactoryIntegrationTests.cs @@ -32,6 +32,15 @@ public void GetDefaultWebDriverShouldReturnADriverProxyWithIdentification() Assert.That(() => driver.WebDriver.GetBrowserId(), Is.Not.Null); } + [Test] + public void GetWebDriverWithOptionsShouldNotThrow() + { + var services = GetServiceProvider(o => o.SelectedConfiguration = "FakeWithOptions"); + var driverFactory = services.GetRequiredService(); + using var driver = driverFactory.GetDefaultWebDriver(); + Assert.That(() => driver.WebDriver.GetBrowserId(), Is.Not.Null); + } + [Test] public void DriverTypeNorOptionsTypeShouldBeMandatoryIfACustomFactoryTypeIsSpecified() { @@ -108,4 +117,12 @@ public WebDriverAndOptions GetWebDriver(WebDriverCreationOptions options, Action return new(Mock.Of(), Mock.Of()); } } + + public class FakeOptionsUsingWebDriverFactory : ICreatesWebDriverFromOptions + { + public WebDriverAndOptions GetWebDriver(WebDriverCreationOptions options, Action? supplementaryConfiguration = null) + { + return new(Mock.Of(), options.OptionsFactory()); + } + } } \ No newline at end of file diff --git a/CSF.Extensions.WebDriver.Tests/Factories/WebDriverFromThirdPartyFactoryTests.cs b/CSF.Extensions.WebDriver.Tests/Factories/WebDriverFromThirdPartyFactoryTests.cs index 42499dc..9d60755 100644 --- a/CSF.Extensions.WebDriver.Tests/Factories/WebDriverFromThirdPartyFactoryTests.cs +++ b/CSF.Extensions.WebDriver.Tests/Factories/WebDriverFromThirdPartyFactoryTests.cs @@ -106,6 +106,24 @@ public void GetWebDriverShouldCustomiseDriverOptionsWithCallbackWhenItIsSpecifie Assert.That(driverOptions.ToCapabilities()["Foo"], Is.EqualTo("Bar")); } + [Test,AutoMoqData] + public void GetWebDriverShouldThrowInvalidOperationExceptionIfOptionsFactoryIsUnset( + [StandardTypes] IGetsWebDriverAndOptionsTypes typeProvider, + [Frozen] IServiceProvider services, + WebDriverFromThirdPartyFactory sut) + { + var options = new WebDriverCreationOptions + { + DriverType = nameof(RemoteWebDriver), + GridUrl = "nonsense://127.0.0.1:1/nonexistent/path", + DriverFactoryType = typeof(FakeWebDriverFactory).AssemblyQualifiedName, + }; + Mock.Get(services).Setup(x => x.GetService(typeof(FakeWebDriverFactory))).Returns(() => new FakeWebDriverFactory()); + Mock.Get(typeProvider).Setup(x => x.GetWebDriverFactoryType(typeof(FakeWebDriverFactory).AssemblyQualifiedName)).Returns(typeof(FakeWebDriverFactory)); + + Assert.That(() => sut.GetWebDriver(options, null), Throws.InvalidOperationException); + } + public class FakeWebDriverFactory : ICreatesWebDriverFromOptions { public WebDriverAndOptions GetWebDriver(WebDriverCreationOptions options, Action? supplementaryConfiguration = null) diff --git a/CSF.Extensions.WebDriver.Tests/appsettings.WebDriverFactoryIntegrationTests.json b/CSF.Extensions.WebDriver.Tests/appsettings.WebDriverFactoryIntegrationTests.json index 34e6f90..b078523 100644 --- a/CSF.Extensions.WebDriver.Tests/appsettings.WebDriverFactoryIntegrationTests.json +++ b/CSF.Extensions.WebDriver.Tests/appsettings.WebDriverFactoryIntegrationTests.json @@ -18,6 +18,12 @@ "OptionsType": "ChromeOptions", "GridUrl": "nonsense://127.0.0.1/no-http-request/should-be-made" }, + "FakeWithOptions": { + "DriverType": "RemoteWebDriver", + "OptionsType": "ChromeOptions", + "GridUrl": "invalid://127.0.0.1/no-http-request/should-be-made", + "DriverFactoryType": "CSF.Extensions.WebDriver.Factories.WebDriverFactoryIntegrationTests+FakeOptionsUsingWebDriverFactory, CSF.Extensions.WebDriver.Tests" + }, "OmittedDriverAndOptionsType": { "DriverFactoryType": "CSF.Extensions.WebDriver.Factories.WebDriverFactoryIntegrationTests+FakeWebDriverFactory, CSF.Extensions.WebDriver.Tests" }, diff --git a/CSF.Extensions.WebDriver/Factories/WebDriverConfigurationItemParser.cs b/CSF.Extensions.WebDriver/Factories/WebDriverConfigurationItemParser.cs index 2d01a0e..7b657ab 100644 --- a/CSF.Extensions.WebDriver/Factories/WebDriverConfigurationItemParser.cs +++ b/CSF.Extensions.WebDriver/Factories/WebDriverConfigurationItemParser.cs @@ -61,16 +61,15 @@ public WebDriverCreationOptions GetDriverConfiguration(IConfigurationSection con bool TryGetDriverType(WebDriverCreationOptions options, IConfigurationSection configuration, out Type driverType) { driverType = null; - if(options.DriverFactoryType != null) - return true; if(options.DriverType is null) { - logger.LogError("{ParamName} is mandatory unless {FactoryTypeKey} is specified; the configuration '{ConfigKey}' will be omitted.", - nameof(WebDriverCreationOptions.DriverType), - nameof(WebDriverCreationOptions.DriverFactoryType), - configuration.Key); - return false; + if(options.DriverFactoryType == null) + logger.LogError("{ParamName} is mandatory unless {FactoryTypeKey} is specified; the configuration '{ConfigKey}' will be omitted.", + nameof(WebDriverCreationOptions.DriverType), + nameof(WebDriverCreationOptions.DriverFactoryType), + configuration.Key); + return options.DriverFactoryType != null ? true : false; } try @@ -80,14 +79,15 @@ bool TryGetDriverType(WebDriverCreationOptions options, IConfigurationSection co } catch(Exception e) { - logger.LogError(e, - "No implementation of {WebDriverIface} could be found for the {DriverTypeProp} '{DriverType}'; the driver configuration '{ConfigKey}' will be omitted. " + - "Reminder: If the driver type is not one which is shipped with Selenium then you must specify its assembly-qualified type name.", - nameof(IWebDriver), - nameof(WebDriverCreationOptions.DriverType), - options.DriverType, - configuration.Key); - return false; + if(options.DriverFactoryType == null) + logger.LogError(e, + "No implementation of {WebDriverIface} could be found for the {DriverTypeProp} '{DriverType}'; the driver configuration '{ConfigKey}' will be omitted. " + + "Reminder: If the driver type is not one which is shipped with Selenium then you must specify its assembly-qualified type name.", + nameof(IWebDriver), + nameof(WebDriverCreationOptions.DriverType), + options.DriverType, + configuration.Key); + return options.DriverFactoryType != null ? true : false; } } @@ -110,8 +110,6 @@ bool TryGetDriverType(WebDriverCreationOptions options, IConfigurationSection co bool TryGetOptionsType(WebDriverCreationOptions options, IConfigurationSection configuration, Type driverType, out Type optionsType) { optionsType = null; - if(options.DriverFactoryType != null) - return true; try { @@ -119,16 +117,18 @@ bool TryGetOptionsType(WebDriverCreationOptions options, IConfigurationSection c } catch(Exception e) { - logger.LogError(e, - "No type deriving from {OptionsBase} could be found for the combination of {WebDriverIface} {DriverType} and {OptionsTypeProp} '{OptionsType}'; the configuration '{ConfigKey}' will be omitted. " + - "See the exception details for more information.", - nameof(DriverOptions), - nameof(IWebDriver), - driverType.Name, - nameof(WebDriverCreationOptions.OptionsType), - options.OptionsType, - configuration.Key); - return false; + if(options.DriverFactoryType == null) + logger.LogError(e, + "No type deriving from {OptionsBase} could be found for the combination of {WebDriverIface} {DriverType} and {OptionsTypeProp} '{OptionsType}'; the configuration '{ConfigKey}' will be omitted. " + + "See the exception details for more information.", + nameof(DriverOptions), + nameof(IWebDriver), + driverType?.Name, + nameof(WebDriverCreationOptions.OptionsType), + options?.OptionsType, + configuration.Key); + + return options.DriverFactoryType != null ? true : false; } try @@ -138,12 +138,13 @@ bool TryGetOptionsType(WebDriverCreationOptions options, IConfigurationSection c } catch(Exception e) { - logger.LogError(e, - "An unexpected error occurred creating or binding to the {OptionsClass} type {OptionsType}; the configuration '{ConfigKey}' will be omitted.", - nameof(DriverOptions), - optionsType.FullName, - configuration.Key); - return false; + if(options.DriverFactoryType == null) + logger.LogError(e, + "An unexpected error occurred creating or binding to the {OptionsClass} type {OptionsType}; the configuration '{ConfigKey}' will be omitted.", + nameof(DriverOptions), + optionsType.FullName, + configuration.Key); + return options.DriverFactoryType != null ? true : false; } } diff --git a/CSF.Extensions.WebDriver/Factories/WebDriverCreationOptions.cs b/CSF.Extensions.WebDriver/Factories/WebDriverCreationOptions.cs index c8df0f6..f07cff9 100644 --- a/CSF.Extensions.WebDriver/Factories/WebDriverCreationOptions.cs +++ b/CSF.Extensions.WebDriver/Factories/WebDriverCreationOptions.cs @@ -1,6 +1,7 @@ using System; using CSF.Extensions.WebDriver.Proxies; using OpenQA.Selenium; +using OpenQA.Selenium.DevTools; namespace CSF.Extensions.WebDriver.Factories { @@ -16,6 +17,8 @@ namespace CSF.Extensions.WebDriver.Factories /// public class WebDriverCreationOptions { + Func optionsFactory = GetUnsetOptionsFactory(); + /// /// Gets or sets a value indicating the concrete name of the class which should be used /// as the implementation. @@ -134,15 +137,23 @@ public class WebDriverCreationOptions /// See the documentation for for more information /// /// - /// If the is in-use then this configuration property is generally unused, because the - /// specified factory is expected to take full control over the options creation. It is particularly unusual to specify + /// If the is in-use then this configuration property is often unused, because the + /// specified factory usually takes full control over the options creation. It is particularly unusual to specify /// this property in that scenario, because doing so would also require specifying either or both of /// and , which are also typically unused when a custom factory is specified. /// If it is specified, then the value will be provided to the custom factory, but the factory is under no obligation to /// use or respect its value. /// + /// + /// Note that if neither or are specified then this property will default + /// to a function which always throws , with a message indicating that it may be not be used. + /// /// - public Func OptionsFactory { get; set; } + public Func OptionsFactory + { + get => optionsFactory; + set => optionsFactory = value ?? GetUnsetOptionsFactory(); + } /// /// An optional object which implements for the corresponding @@ -264,5 +275,10 @@ public class WebDriverCreationOptions /// /// public bool AddBrowserQuirks { get; set; } = true; + + static Func GetUnsetOptionsFactory() + { + return () => throw new InvalidOperationException($"Driver options cannot be created via {nameof(OptionsFactory)}; either {nameof(DriverType)} must be set to a type which indicates a deterministic options type or {nameof(OptionsType)} must set set. If you are using a custom {nameof(DriverFactoryType)} then it may not be appropriate to create options in this way."); + } } } \ No newline at end of file