Skip to content

Conversation

michaelnebel
Copy link
Contributor

@michaelnebel michaelnebel commented Nov 29, 2021

In this PR we use a combination of auto generated and (existing) hand written stubs as a replacement for dll's in the flow summaries test.
We suspect that the rows that have been removed from the flow summaries are due to the fact that the summaries are now created based on Microsoft.NETCore.App version 2.2.8, which is from 2019, where as some of the dll's that it is replacing might be newer.
On the other hand more summaries have been introduced than removed.
It would be great, if we could have a project for using NETCore 6.0 as the basis for our stubs.

As a side effect, some cleanup was needed in the hand written stub System.Web.cs as we otherwise would have an overlapping implementation of the class HttpResponse (as it exists in Microsoft.NETCore.App). This has a rippling effect into other test cases, which relies on System.Web.cs. These have also been cleaned up and refers now only to auto generated and manually created stubs (this required autogenerating more stubs).

@github-actions github-actions bot added the C# label Nov 29, 2021
@michaelnebel michaelnebel marked this pull request as ready for review November 30, 2021 15:48
@michaelnebel michaelnebel requested a review from a team as a code owner November 30, 2021 15:48
@hvitved
Copy link
Contributor

hvitved commented Dec 1, 2021

We suspect that the rows that have been removed from the flow summaries are due to the fact that the summaries are now created based on Microsoft.NETCore.App version 2.2.8, which is from 2019, where as some of the dll's that it is replacing might be newer.

Didn't we end up basing it on the .NET 5 SDK?

@@ -0,0 +1,12 @@
<Project Sdk="Microsoft.NET.Sdk">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we getting just .csproj files, and no .cs files? Perhaps @tamasvajk knows?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can remove the folders that contain only a .csproj file, but we need to also remove the reference to these .csproj files.

We probably end up with these projects, because the original assemblies didn't contain any public types. As far as I remember, we're creating the folder structure based on the project.assets.json file (and not the extracted types), which includes these assemblies too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you prefer that I manually go through the generated stubs and remove what is strictly not needed?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case let's just leave them in.

@michaelnebel
Copy link
Contributor Author

We suspect that the rows that have been removed from the flow summaries are due to the fact that the summaries are now created based on Microsoft.NETCore.App version 2.2.8, which is from 2019, where as some of the dll's that it is replacing might be newer.

Didn't we end up basing it on the .NET 5 SDK?

The same stubs are generated for Microsoft.NETCore.App (module some comments mentioning NewtonSoft.JSon), when stubs are generated for NewtonSoft.JSon using net5.0 as the target framework compared to generating the stubs directly for Microsoft.NETCore.App version 2.2.8.

@michaelnebel michaelnebel merged commit 75f9a94 into github:main Dec 1, 2021
@michaelnebel michaelnebel deleted the csharp-nuget-packages branch December 1, 2021 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants