[dotnet] Create separation of concerns #110
Conversation
titusfortner
left a comment
titusfortner
left a comment
There was a problem hiding this comment.
I didn't spend a lot of time looking at this versus master, and I just knocked out a few ideas for how to split up these classes in the last two commits on this branch: https://github.com/saucelabs/simple_sauce/commits/dotnet_titus
I only got so far on that code before I stopped. Let me know if you want me to do more with it.
dotnet/Simple.Sauce/SauceOptions.cs
Outdated
| private readonly Dictionary<string, object> _sauceConfiguration; | ||
|
|
||
| public SauceOptions() | ||
| public SauceOptions() : this(new DriverFactory()) |
There was a problem hiding this comment.
The biggest issue is that Options shouldn't know anything about the Driver.
| } | ||
|
|
||
| public SauceSession(SauceOptions options, ISauceRemoteDriver driver) | ||
| public SauceSession(SauceOptions options, IWebDriver driver) |
There was a problem hiding this comment.
Session is responsible for creating a driver session, not be initialized with a driver instance.
There was a problem hiding this comment.
Agreed. The Java case of SauceOptions doesn't allow any form of WebDriver to be passed in to constructors or getters. This should be the same in the .NET case.
| public EdgeOptions ConfiguredEdgeOptions { get; set; } | ||
| public ChromeOptions ConfiguredChromeOptions { get; private set; } | ||
| public SafariOptions ConfiguredSafariOptions { get; set; } | ||
| public DataCenter DataCenter { get; set; } = DataCenter.UsWest; |
There was a problem hiding this comment.
Data Center is a Session concern not an Options concern
dotnet/Simple.Sauce/SauceOptions.cs
Outdated
| } | ||
|
|
||
| public IDriverFactory DriverFactory { get; } | ||
| public EdgeOptions ConfiguredEdgeOptions { get; set; } |
There was a problem hiding this comment.
we shouldn't need to have separate configurations for each browser implementation. They are all type DriverOptions, we just need to be able to get the formatting to the Session when asked.
| } | ||
|
|
||
| public SauceSession(SauceOptions options, ISauceRemoteDriver driver) | ||
| public SauceSession(SauceOptions options, IWebDriver driver) |
There was a problem hiding this comment.
Agreed. The Java case of SauceOptions doesn't allow any form of WebDriver to be passed in to constructors or getters. This should be the same in the .NET case.
dotnet/Simple.Sauce/SauceOptions.cs
Outdated
| }; | ||
| } | ||
|
|
||
| public IWebDriver CreateEdgeBrowser() |
There was a problem hiding this comment.
Where's the Firefox case?
There was a problem hiding this comment.
To expound on this, we shouldn't need anything that is specific to a browser here. For Typing we should be able to use the superclass.
|
Issues
======
- Added 4
Complexity increasing per file
==============================
- dotnet/SimpleSauce.Test/SauceOptionsTests.cs 1
- dotnet/SimpleSauce.Test/DriverFactoryTests.cs 1
- dotnet/Simple.Sauce/DriverFactory.cs 4
Complexity decreasing per file
==============================
+ dotnet/Simple.Sauce/SauceSession.cs -1
Clones added
============
- dotnet/SimpleSauce.Test/SauceOptionsTests.cs 1
- dotnet/SimpleSauce.Test/DriverFactoryTests.cs 1
- dotnet/SimpleSauce.Test/SauceSessionTests.cs 1
Clones removed
==============
+ dotnet/SimpleSauce.Test/BaselineSeleniumAcceptanceTests.cs -1
+ dotnet/SimpleSauce.Test/SimpleSauceAcceptanceTests.cs -1
See the complete overview on Codacy |
| Options = new SauceOptions(); | ||
| } | ||
| private IWebDriver _driver; | ||
| private IDriverFactory _driverFactory; |
There was a problem hiding this comment.
| Driver = new SauceDriver(); | ||
| Options = new SauceOptions(); | ||
| } | ||
| private IWebDriver _driver; |
| { | ||
| private readonly string _sauceUserName = Environment.GetEnvironmentVariable("SAUCE_USERNAME"); | ||
| private readonly string _sauceAccessKey = Environment.GetEnvironmentVariable("SAUCE_ACCESS_KEY"); | ||
| public readonly Dictionary<string, object> _sauceConfiguration; |
There was a problem hiding this comment.
| { | ||
| private Mock<ISauceRemoteDriver> _dummyDriver; | ||
| private Mock<IDriverFactory> _driverFactory; | ||
| private Mock<SauceOptions> _mockOptions; |
There was a problem hiding this comment.
I've updated the C# implementation to match what we have been doing in Java. This is a pretty good preview of where the bindings can go. Please let me know your thoughts.