dotnet-svcutil test update: remove unsupported .net version tests and… #5042

imcarolwang · 2023-03-16T02:28:02Z

… fixing baselines.

To make changes of this PR clear, split it into following three:
#5050: remove CloseAsync()
#5051: remove unsupported framework targets
#5052: fix ParamsFiles and typereuse cases which hard code to net6.0

… fixing baselines.

HongGit

please documents all the changes in the description to make this PR clear
does it make sense to separate the CloseAsync in a separate PR?
I see making the version generic is more dangerous than the gain, particularly in the type reuse side, what do you think? I would suggest to make this PR purely remove the old non-supported framework PR. We could rethink about the future, whether to make it generic or not.
I also think we need to throw an error message when the targeted app is on non-supported target framework.

HongGit · 2023-03-17T02:57:33Z

Directory.Build.props

@@ -25,8 +25,8 @@
  </PropertyGroup>

  <PropertyGroup>
-    <ScenarioTestTargetFrameworks>net6.0;net7.0</ScenarioTestTargetFrameworks>
-	<UnitTestTargetFrameworks>net6.0;net7.0</UnitTestTargetFrameworks>
+    <ScenarioTestTargetFrameworks>net6.0;net8.0</ScenarioTestTargetFrameworks>


why drop net7.0?

I meant to upgrade net7.0 to net8.0 to accommodate the .net sdk and runtime definitions in https://github.com/dotnet/wcf/blob/main/global.json

I found this setting only affects WCF tests, not dotnet-svcutil tests. So it's not changed in the newly created separated PR.

HongGit · 2023-03-17T02:58:37Z

src/dotnet-svcutil/lib/tests/Baselines/Collections/array/Reference.cs

@@ -889,11 +889,6 @@ public virtual System.Threading.Tasks.Task OpenAsync()
            return System.Threading.Tasks.Task.Factory.FromAsync(((System.ServiceModel.ICommunicationObject)(this)).BeginOpen(null, null), new System.Action<System.IAsyncResult>(((System.ServiceModel.ICommunicationObject)(this)).EndOpen));
        }

-        public virtual System.Threading.Tasks.Task CloseAsync()


Is there any risk of removing this? Should another test be added?

In addition, can you please put a summary of all changes as it will make it easier to understand the changes.

The test baselines are updated for changes of PR #4909 (issue #4891), dotnet-svcutil no longer generate CloseAsync() when the referenced wcf package version equals or greater than 4.10.

The risk here is we missed testing the tool running in projects with different .net targets, it is due to the limitation of .net defined in global.json under the repository root. We do cover this scenario in manual testing, specifically, verifying that when the test project references wcf packages older than 4.10.*, the CloseAysnc() still get generated.

HongGit · 2023-03-17T03:07:47Z

src/dotnet-svcutil/lib/tests/Baselines/ParamsFiles_SDK_TFM/_edb/_edb.csproj

@@ -3,6 +3,8 @@
  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>N.N</TargetFramework>
+    <ImplicitUsings>enable</ImplicitUsings>


should we still support coreapp2.0 when we are deleting netcoreapp3.1?

HongGit · 2023-03-17T03:09:17Z

src/dotnet-svcutil/lib/tests/Baselines/ParamsFiles_SDK_TFM/_edb/_edb.params.json

@@ -1,18 +1,18 @@
 {
-  "providerId": "TFnet5_0",
+  "providerId": "_edb",


why not use 6.0 to replace 5.0? Is _edb defined somewhere, I might have missed it>

HongGit · 2023-03-17T03:11:14Z

src/dotnet-svcutil/lib/tests/Baselines/TypeReuse/TypeReuse60/SvcutilBootstrapper/Program.cs

is this part of this change? From the name, sound like this is used for net6.0? Is that not the case?

HongGit · 2023-03-17T03:13:51Z

src/dotnet-svcutil/lib/tests/Baselines/TypeReuse/TypeReuseDefault/ServiceReference/Reference.cs

@@ -7,12 +7,12 @@
 // </auto-generated>
 //------------------------------------------------------------------------------

-namespace TypeReuse60_NS


I see that you are making this generic. How does this impact combability?

In addition, perhaps we should throw an erro message when working with version less than 5.0, what do you think?

Does generic will make it work with all versions of target Framework? We do need to think about this.

HongGit · 2023-03-17T03:16:13Z

src/dotnet-svcutil/lib/tests/Baselines/WcfRuntimeNetHttpSvcs/NetHttps/Reference.cs

        [System.ServiceModel.FaultContractAttribute(typeof(NetHttps_NS.FaultDetail), Action="http://tempuri.org/IWcfService/TestFaultFaultDetailFault", Name="FaultDetail", Namespace="http://www.contoso.com/wcfnamespace")]
+        [System.ServiceModel.FaultContractAttribute(typeof(NetHttps_NS.FaultDetail), Action="http://tempuri.org/IWcfService/TestFaultFaultDetailFault2", Name="FaultDetail2", Namespace="http://www.contoso.com/wcfnamespace")]


sorry, I am not sure I know what this is for.

In generated code, the attributes' order is different than originally generated, looks relate to CodeDom other than bug from the tool.

HongGit · 2023-03-17T03:18:54Z

src/dotnet-svcutil/lib/tests/src/E2ETests.cs

@@ -204,12 +201,12 @@ public void NamespaceParam(string testCaseName, string options, bool expectSucce

        [Trait("Category", "BVT")]
        [Theory]
-        [InlineData("TypeReuse60", "net6.0")]


rather than use a generic format, I do prefer to add net7.0 and net 8.0 here to make it explicitly clear. However, I do see your point that we don't need to update this in the future. Similar to another comment before, does this mean we also allow other earlier .net versions? We do need to be careful here.

The format is made generic because the .net version is actually not used at all in the test, in the case context it always creates a test.csproj targets to the latest .net version (currently it's net8.0 as defined in global.json) to test type-reuse feature.

HongGit · 2023-03-17T03:19:14Z

src/dotnet-svcutil/lib/tests/src/GlobalToolTests.cs

        {
            this_TestCaseName = "ParamsFiles_SDK_TFM";
            TestFixture();
-            var testCaseName = $"TF{targetFramework}".Replace(".", "_");


same as above

imcarolwang · 2023-03-21T06:33:06Z

please documents all the changes in the description to make this PR clear

does it make sense to separate the CloseAsync in a separate PR?

I see making the version generic is more dangerous than the gain, particularly in the type reuse side, what do you think? I would suggest to make this PR purely remove the old non-supported framework PR. We could rethink about the future, whether to make it generic or not.

I also think we need to throw an error message when the targeted app is on non-supported target framework.

Thanks for your review!

I think it makes sense to put the CloseAsync in a separate PR, I will keep this one open as reference.
I'll make this PR purely remove the old non-supported framework to make it clearer.
As regarding to make versions generic for 2 cases which hard coded to .net6.0, they currently fail to run because the test code can only create project targeting to .net8.0, I will make the change in new PR and work on it from there.

dotnet-svcutil test update: remove unsupported .net version tests and…

28843cd

… fixing baselines.

HongGit reviewed Mar 17, 2023

View reviewed changes

imcarolwang added the PR DO NOT MERGE If for whatever reason you do not want a PR merged. label Mar 21, 2023

imcarolwang closed this Dec 4, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dotnet-svcutil test update: remove unsupported .net version tests and… #5042

dotnet-svcutil test update: remove unsupported .net version tests and… #5042

imcarolwang commented Mar 16, 2023 •

edited

Loading

HongGit left a comment

HongGit Mar 17, 2023

imcarolwang Mar 17, 2023

imcarolwang Mar 21, 2023 •

edited

Loading

HongGit Mar 17, 2023

imcarolwang Mar 17, 2023

HongGit Mar 17, 2023

HongGit Mar 17, 2023

HongGit Mar 17, 2023

HongGit Mar 17, 2023

HongGit Mar 17, 2023

HongGit Mar 17, 2023

imcarolwang Mar 17, 2023 •

edited

Loading

HongGit Mar 17, 2023

imcarolwang Mar 21, 2023

HongGit Mar 17, 2023

imcarolwang commented Mar 21, 2023

		[System.ServiceModel.FaultContractAttribute(typeof(NetHttps_NS.FaultDetail), Action="http://tempuri.org/IWcfService/TestFaultFaultDetailFault", Name="FaultDetail", Namespace="http://www.contoso.com/wcfnamespace")]
		[System.ServiceModel.FaultContractAttribute(typeof(NetHttps_NS.FaultDetail), Action="http://tempuri.org/IWcfService/TestFaultFaultDetailFault2", Name="FaultDetail2", Namespace="http://www.contoso.com/wcfnamespace")]

dotnet-svcutil test update: remove unsupported .net version tests and… #5042

dotnet-svcutil test update: remove unsupported .net version tests and… #5042

Conversation

imcarolwang commented Mar 16, 2023 • edited Loading

HongGit left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imcarolwang Mar 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imcarolwang Mar 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imcarolwang commented Mar 21, 2023

imcarolwang commented Mar 16, 2023 •

edited

Loading

imcarolwang Mar 21, 2023 •

edited

Loading

imcarolwang Mar 17, 2023 •

edited

Loading