-
Notifications
You must be signed in to change notification settings - Fork 188
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
refactor: Use netstandard2.0
as target framework to support wider frameworks
#845
Changes from all commits
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 |
---|---|---|
@@ -1,14 +1,14 @@ | ||
<Project Sdk="Microsoft.NET.Sdk"> | ||
<PropertyGroup> | ||
<TargetFrameworks>net48;net6.0</TargetFrameworks> | ||
<PackageLicenseFile>LICENSE.txt</PackageLicenseFile> | ||
<TargetFrameworks>netstandard2.0</TargetFrameworks> | ||
<RootNamespace>OpenQA.Selenium</RootNamespace> | ||
<LangVersion>latest</LangVersion> | ||
<Company>Appium Commiters</Company> | ||
<Product>Dotnet-Client</Product> | ||
<PackageRequireLicenseAcceptance>false</PackageRequireLicenseAcceptance> | ||
<GeneratePackageOnBuild>true</GeneratePackageOnBuild> | ||
<Copyright>Copyright © 2023</Copyright> | ||
<PackageProjectUrl>https://github.com/appium/dotnet-client</PackageProjectUrl> | ||
<PackageProjectUrl>https://appium.io</PackageProjectUrl> | ||
<RepositoryUrl>https://github.com/appium/dotnet-client</RepositoryUrl> | ||
<RepositoryType>git</RepositoryType> | ||
<IncludeSymbols>true</IncludeSymbols> | ||
|
@@ -19,40 +19,25 @@ | |
<PackageIcon>appium-icon.png</PackageIcon> | ||
<Description>Selenium Webdriver extension for Appium.</Description> | ||
<PackageTags>Appium Webdriver device automation</PackageTags> | ||
</PropertyGroup> | ||
<PropertyGroup> | ||
<PackageLicenseFile>LICENSE.txt</PackageLicenseFile> | ||
</PropertyGroup> | ||
<PropertyGroup> | ||
<PackageReadmeFile>README.md</PackageReadmeFile> | ||
</PropertyGroup> | ||
<PropertyGroup> | ||
<GenerateDocumentationFile>true</GenerateDocumentationFile> | ||
</PropertyGroup> | ||
<PropertyGroup> | ||
<LangVersion>latest</LangVersion> | ||
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. Shouldn't be we keep 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. Removed here, added upper. |
||
</PropertyGroup> | ||
|
||
<PropertyGroup Condition="'$(Configuration)|$(TargetFramework)|$(Platform)'=='Debug|netstandard2.0|AnyCPU'"> | ||
<NoWarn>1701;1702;1591</NoWarn> | ||
</PropertyGroup> | ||
|
||
<PropertyGroup Condition="'$(Configuration)|$(TargetFramework)|$(Platform)'=='Release|netstandard2.0|AnyCPU'"> | ||
<NoWarn>1701;1702;1591</NoWarn> | ||
</PropertyGroup> | ||
<PropertyGroup Condition="'$(Configuration)|$(TargetFramework)|$(Platform)'=='Debug|net48|AnyCPU'"> | ||
<NoWarn>1701;1702;1591</NoWarn> | ||
</PropertyGroup> | ||
<PropertyGroup Condition="'$(Configuration)|$(TargetFramework)|$(Platform)'=='Release|net48|AnyCPU'"> | ||
<NoWarn>1701;1702;1591</NoWarn> | ||
</PropertyGroup> | ||
|
||
<ItemGroup> | ||
<None Include="LICENSE.txt" Pack="true" PackagePath="" /> | ||
</ItemGroup> | ||
<ItemGroup> | ||
<None Include="appium-icon.png" Pack="true" PackagePath="" /> | ||
<None Include="..\..\README.md" Pack="true" PackagePath="" /> | ||
</ItemGroup> | ||
<ItemGroup> | ||
<None Include="..\..\README.md" Pack="true" PackagePath="" /> | ||
</ItemGroup> | ||
|
||
<ItemGroup> | ||
<PackageReference Include="Microsoft.SourceLink.GitHub" Version="8.0.0"> | ||
<PrivateAssets>all</PrivateAssets> | ||
|
@@ -62,8 +47,4 @@ | |
<PackageReference Include="Selenium.WebDriver" Version="4.24.0" /> | ||
<PackageReference Include="System.Drawing.Common" Version="8.0.8" /> | ||
</ItemGroup> | ||
<ItemGroup> | ||
<Folder Include="Properties\" /> | ||
<Folder Include="Appium\Mac\" /> | ||
</ItemGroup> | ||
</Project> | ||
</Project> |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,9 +20,7 @@ | |
using System.IO; | ||
using System.Linq; | ||
using System.Net; | ||
#if NET | ||
using System.Net.Http; | ||
#endif | ||
using System.Runtime.CompilerServices; | ||
using System.Threading.Tasks; | ||
|
||
|
@@ -39,9 +37,7 @@ public class AppiumLocalService : ICommandServer | |
private readonly int Port; | ||
private readonly TimeSpan InitializationTimeout; | ||
private readonly IDictionary<string, string> EnvironmentForProcess; | ||
#if !NET48 | ||
private readonly HttpClient SharedHttpClient; | ||
#endif | ||
private Process Service; | ||
private List<string> ArgsList; | ||
|
||
|
@@ -65,24 +61,14 @@ internal AppiumLocalService( | |
Port = port; | ||
InitializationTimeout = initializationTimeout; | ||
EnvironmentForProcess = environmentForProcess; | ||
#if !NET48 | ||
SharedHttpClient = CreateHttpClientInstance; | ||
#endif | ||
SharedHttpClient = CreateHttpClientInstance(); | ||
} | ||
|
||
#if !NET48 | ||
private HttpClient CreateHttpClientInstance | ||
Comment on lines
-73
to
-74
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. @nvborisenko have you run some tests against .NET framework 4.7+? 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. Do you know any known issues? 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. Only .net framework 4.7 is under interest? 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. You said you were trying to run with. Net 4.7, so this is why I asked. 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.
Should be the same issues I had for 4.8 😁 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 not from Selenium team, I am contributing as personal. 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. And you will be surprised if you look at dependencies of Playwright. 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. Ok. Let's go ahead with this change. 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. Selenium is already felt it. |
||
private static HttpClient CreateHttpClientInstance() | ||
{ | ||
get | ||
{ | ||
SocketsHttpHandler handler = new SocketsHttpHandler | ||
{ | ||
PooledConnectionLifetime = TimeSpan.FromMinutes(2) | ||
}; | ||
return new HttpClient(handler); | ||
} | ||
return new HttpClient() { Timeout = TimeSpan.FromMinutes(2) }; | ||
} | ||
#endif | ||
|
||
/// <summary> | ||
/// The base URL for the managed appium server. | ||
/// </summary> | ||
|
@@ -175,9 +161,7 @@ private void DestroyProcess() | |
finally | ||
{ | ||
Service?.Close(); | ||
#if !NET48 | ||
SharedHttpClient.Dispose(); | ||
#endif | ||
} | ||
} | ||
|
||
|
@@ -296,20 +280,12 @@ private async Task<bool> PingAsync(TimeSpan span) | |
{ | ||
try | ||
{ | ||
#if NET48 | ||
HttpWebResponse response = await GetHttpResponseAsync(status).ConfigureAwait(false); | ||
if (response.StatusCode == HttpStatusCode.OK) | ||
{ | ||
return true; | ||
} | ||
#elif NET | ||
HttpResponseMessage response = await GetHttpResponseAsync(status).ConfigureAwait(false); | ||
using HttpResponseMessage response = await SharedHttpClient.GetAsync(status).ConfigureAwait(false); | ||
|
||
if (response.IsSuccessStatusCode) | ||
{ | ||
return true; | ||
} | ||
#endif | ||
} | ||
catch | ||
{ | ||
|
@@ -318,21 +294,5 @@ private async Task<bool> PingAsync(TimeSpan span) | |
} | ||
return pinged; | ||
} | ||
#if NET48 | ||
private async Task<HttpWebResponse> GetHttpResponseAsync(Uri status) | ||
{ | ||
return await Task.Run(() => | ||
{ | ||
HttpWebRequest request = (HttpWebRequest)WebRequest.Create(status); | ||
return (HttpWebResponse)request.GetResponse(); | ||
}).ConfigureAwait(false); | ||
} | ||
#else | ||
private async Task<HttpResponseMessage> GetHttpResponseAsync(Uri status) | ||
{ | ||
HttpResponseMessage response = await SharedHttpClient.GetAsync(status).ConfigureAwait(false); | ||
return response; | ||
} | ||
#endif | ||
} | ||
} |
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.
Why was this removed?
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.
Removed and added
netstandard2.0
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.
If you mean
licence.txt
, then it was duplicated.