-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Delete tests specific to .NET Framework? #28619
Comments
This sounds reasonable. |
No objections from me. |
Confirm often I don't do double testing(also time consuming), also 99% of time I fail on UAP, I don't know if the story is the same. |
@MarcoRossignoli note that this first step won't help much with that because tests will still run on both frameworks by default (although we might change that later, as mentioned above). For now this is proposing just getting rid of the tests that only apply to .NET Framework. @jkotas ? @davidsh @geoffkizer thoughts for networking? |
Oh, I see @jkotas already did thumbs up 👍 |
This makes sense for components that ship in .NET Core Runtime where .NET Framework is not shipping config and we run the tests on .NET Framework just to check for parity. We should continue to maintain .NET Framework specific paths coverage for components where .NET Framework is a shipping config. Examples of such components are |
What about OOB packages with a .NET Standard <= 2.0 profile (consumable by .NET Framework)? |
.NET Framework is a shipping config for these. We should maintain the test coverage. |
This is the case for System.Net.Http.WinHttpHandler which is a separate NuGet package that ships on .NET Core and .NET Framework. While SocketsHttpHandler is the new default for the underlying HTTP stack of HttpClientHandler, WinHttpHandler still ships separately to address .NET Framework customers that need HTTP/2 and other features specific to WinHttpHandler. As part of supporting that, we also use the broad set of Http tests in System.Net.Http library to run against .NET Core and .NET Framework. And we have ways of invoking WinHttpHandler instead of SocketsHttpHandler to run against for those tests. I don't we are ready to see this test capability go away just yet. |
Up for grabs. If anyone in the community is interested in having a go at this, please let me know so that I can make sure you don't waste time on the libraries we don't want to change (the ones that ship downlevel) |
@danmosemsft I'm the guy from the community who is interested to help with that. Please, tell what libraries you want to clean up. |
Me too 🙋♂️ |
@kislovs @kmhigashioka welcome. You are looking for
Tests are marked to only run on .NET Framework using attributes like this
Tests designate a path that only applies to .NET Framework using conditions based on To mark a test to explicitly not run on .NET Framework (remember, we will still by default run our tests on .NET Framework, we are just removing .NET Framework-specific tests and test codepaths) use
Feel free to preserve any useful comments for 2 and 3 above. For all this effort we want to exclude certain libraries that are still relevant to .NET Framework. Let me define those below. |
@ericstj what is the simplest way to designate the libraries that are still shipped for .NET Framework? |
I think just looking at Configuration.props in the pkg folder should be enough. Check if netfx|net\d{2,3} is present. |
Or do you also want to find packages that harvest netfx resources? I would not recommend including those to the exclusion list as they will stay in the package infinitely. You probably already thought about that but please don't forget to include packages with a <= netstandard2.0 configuration to the exclusion list. |
Should be Another way that's more programmatic is to build the |
To test it, we should actually build it in the netfx vertical, so only values that would apply, would be netstandard/netfx right? If not we can't run its tests against netfx, right? |
Yeah since you're only looking for a true/false that'd work. You can just do a netfx vertical build then examine the runtime folder (F:\dotnet\corefx2\artifacts\bin\runtime\netfx-Windows_NT-Debug-x64), omitting stuff that was copied instead of built. |
I'm trying to figure what that means in concrete terms. I built |
I don't think you need to look under <Target Name="GetBuiltItemsNetfx"
Condition="'@(BinPlaceDir)' != '' AND '$(IsSourceProject)' == 'true'"
AfterTargets="BinPlaceFiles">
<MakeDir Directories="E:\repos\corefxCopy\corefx\artifacts\tempNetfx" />
<Copy SourceFiles="@(_BinPlaceItems)"
OverwriteReadOnlyFiles="true"
DestinationFolder="E:\repos\corefxCopy\corefx\artifacts\tempNetfx" />
</Target> Then just ran The output, which I believe are the projects we care about since are the ones that have a
|
Thanks @safern. OK @kmhigashioka @kislovs are you good to go? If you're still interested, please go through libraries in this repo that are NOT in the list above, and follow the guidance I put above. Thanks! That will simplify our tests. |
Also exclude System.Runtime.Serialization.Formatters as that is important for cross-serialization testing. |
@danmosemsft yeah, sure. I will take a look when I will have free time 👍 |
👍 |
Thanks @kislovs @kmhigashioka |
It never was and is never going to be a goal to use CoreFX tests to find bugs in .NET Framework. Really their only value is as documentation, where we intentionally changed behavior, we created two similar tests next to each other. This is an expensive kind of documentation, that runs every day, inevitably fails from time to time (given how many there are, and the fact that .NET Core is more reliable now). It would be enough documentation to have a comment, or the mere fact that a test is excluded from .NET Framework runs.
There is still some value in having .NET Framework runs, and making new tests default to running on both. But there is a cost too, and in future more and more often, new tests will not apply to .NET Framework because new tests will mostly be for new API and new behaviors. Several times a week, someone writes a test that fails in CI only on .NET Framework, because they didn't think to exclude it. It would be nice to change the default, so new tests don't run on .NET Framework, but I expect that would be too fiddly (involve marking all our existing tests somehow). At some point we will probably want to stop the .NET Framework runs entirely, relying on the tests we have built up (and taking care when we change their assertions) but perhaps that's going a little too far for now (?)
Suggestion: make a pass and delete all tests and test branches that are .NET Framework only. Thoughts?
@jkotas @safern @stephentoub @ViktorHofer @ericstj @marek-safar @hughbe
The text was updated successfully, but these errors were encountered: