Skip to content
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

Run dotnet nuget tests on multiple TFMs #3387

Conversation

zivkan
Copy link
Member

@zivkan zivkan commented May 11, 2020

Bug

Fixes:NuGet/Home#9252
Regression: No

  • Last working version:
  • How are we preventing it in future:

Fix

Details:

  • Add new TFM property for testing, and make Dotnet.Integration.Tests use it.
  • Fix up test fixture to copy only one of the SDK folders for testing, the "nearest match" based on the runtime TFM and that SDK TFM.
  • Create helper methods to write global.json in test folders, and change all tests in Dotnet.Integration.Tests to use it.

Testing/Validation

Tests Added: No
Reason for not adding tests: no product code changes, just modifying the test projects.
Validation:

@zivkan zivkan force-pushed the dev-zivkan-xplatVerification-dotnet-integration-tests-multi-tfm branch 2 times, most recently from 89b3201 to 004e436 Compare May 11, 2020 19:06
@heng-liu heng-liu force-pushed the dev-feature-xplatVerification branch 2 times, most recently from 609c2c8 to ed29dd8 Compare May 26, 2020 04:40
@zivkan zivkan force-pushed the dev-zivkan-xplatVerification-dotnet-integration-tests-multi-tfm branch from 004e436 to 7d3c4f5 Compare May 26, 2020 17:22
@heng-liu
Copy link
Contributor

heng-liu commented May 26, 2020

There is one more test PackCommand_DoesNotGenerateOwnersElement at the end of PackCommandTests you need to change ;)
Not sure if it's the best practice to have all the tests changed to add the global.json file.
As it's easy to forget.
If in future someone want to add more tests into Dotnet.Integration.Tests, how do they know which method is the right method to use?(testDirectory = msbuildFixture.CreateTestDirectory() or testDirectory = TestDirectory.Create())

@zivkan
Copy link
Member Author

zivkan commented May 27, 2020

Not sure if it's the best practice to have all the tests changed to add the global.json file.
As it's easy to forget.
If in future someone want to add more tests into Dotnet.Integration.Tests, how do they know which method is the right method to use?(testDirectory = msbuildFixture.CreateTestDirectory() or testDirectory = TestDirectory.Create())

I agree, from an API point of view it's really not good to have similar APIs.

However, TestDirectory.Create() is used by a whole lot of other projects, not just DotNet.Integration.Tests, and it doesn't make sense for any of them to write a global.json file, particularly as getting it wrong will cause tests to fail. These are the projects which I could quickly find usages: NuGet.CommandLine.FuncTest, NuGet.MSSigning.Extensions.FuncTest, NuGet.CommandLine.Test, NuGet.PackageManagement.UI.Test, NuGet.PackageManagement.VisualStudio.Test, NuGet.VisualStudio.Implementation.Test, NuGetConsole.Host.PowerShell.Test, NuGet.Commands.FuncTest, NuGet.Packaging.FuncTest, NuGet.Protocol.FuncTest, Microsoft.Build.NuGetSdkResolver.Test, NuGet.Build.Tasks.Console.Test, NuGet.Build.Tasks.Pack.Test, NuGet.Build.Tasks.Test, NuGet.Commands.Test, NuGet.Common.Test, NuGet.Configuration.Test, NuGet.PackageManagement.Test, NuGet.Packaging.Test, NuGet.ProjectModel.Test, and NuGet.Protocol.Tests.

The requirement to use global.json to lock a test to a specific SDK version is very specific to DotNet.Integration.Tests due to the fact that this is the only test project where we patch a custom .NET SDK folder and want to make sure we run only that SDK.

If you think it's better, I could create an overload, but developers would still need to know whether to call TestDirectory.Create() (everywhere except DotNet.Integration.Tests) or TestDirectory.Create(string) (only in DotNet.Integration.Tests).

Alternatively, we could investigate writing an analyzer that will error when TestDirectory.Create() is used within DotNet.Integration.Tests, and maybe even offer a codefix if it can find a MsbuildIntegrationTestFixture. But this would be a new issue, as I'd need to learn a lot, and therefore need a lot more time, to do it in this PR.

Copy link
Member

@nkolev92 nkolev92 left a comment

Choose a reason for hiding this comment

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

fwiw, I'm with @zivkan on the TestDirectory.Create stuff.

A question or two in the comments.

@heng-liu
Copy link
Contributor

heng-liu commented May 27, 2020

I agree that it's hard to find a "good place" ,as we use various APIs in dotnet integration tests.
I'm not sure if adding it in all the dotnet command methods in MsbuildIntegrationTestFixture will be better than this. (CreateDotnetNewProject, CreateDotnetToolProject,RestoreToolProject,RestoreProjectOrSolution,RunDotnet,PackProjectOrSolution,BuildProject and so on) I suppose all the dotnet integration test will call at least one dotnet command in MsbuildIntegrationTestFixture, but I'm not sure.
The pros is when adding new tests, we won't forget to do something.
The cons is it's not intuitive.

If the above is not as good as the original one, can you pls consider adding some comments on how to differentiate those method? Thanks.

Copy link
Contributor

@heng-liu heng-liu left a comment

Choose a reason for hiding this comment

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

The pros is when adding new tests, we won't forget to do something.

-----Just realized that, it's not true. If we need to add new dotnet command, we still need to remember adding global.json

{
var projectName = "ClassLibrary1";
var workingDirectory = Path.Combine(testDirectory, projectName);
// Act
msbuildFixture.CreateDotnetNewProject(testDirectory.Path, projectName, " classlib");
msbuildFixture.CreateDotnetNewProject(testDirectory.Path, projectName, " classlib -f netstandard2.0");
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice change! So no more adapting for future TFM change :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately the template doesn't allow arbitrary TFMs. On my machine it only accepts net5.0, netstandard2.1 and netstandard2.0. So if a future SDK removes this TFM from the template, our tests will break.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see..Thanks for clarification.

Comment on lines +442 to +443
var reducer = new FrameworkReducer();
var selectedTfm = reducer.GetNearest(sdkTfm, compiledTfms.Keys);
Copy link
Contributor

Choose a reason for hiding this comment

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

Good to know that we can use GetNearest to find the right bin/Debug/TFM folder, to patch the dotnet. So we can get rid of hardcoded TFM and the if statement in selecting TFM folder.

Copy link
Member Author

Choose a reason for hiding this comment

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

We have multiple methods called GetNearest in several types, and I have no idea what the difference is between them, or if I'm using the "right" one, but it seems to work.

Copy link
Contributor

@kartheekp-ms kartheekp-ms left a comment

Choose a reason for hiding this comment

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

I guess my PR to add couple of new dotnet tests is depending on this PR because one new test in my PR fails as NetStandard packages are not available in the local source created by SimpleTestPathContext.

@heng-liu heng-liu self-requested a review May 29, 2020 17:54
@zivkan zivkan merged commit 1250c59 into dev-feature-xplatVerification May 30, 2020
@zivkan zivkan deleted the dev-zivkan-xplatVerification-dotnet-integration-tests-multi-tfm branch May 30, 2020 00:25
aortiz-msft pushed a commit that referenced this pull request Jun 20, 2020
* Signing: Support signature verification on .NET Core

Remove 2nd N.B.T.P package (#2984)

remove WorkaroundNetStandard target, change SDK for build from 3.0 preview to 3.1 stable (#3161)

* remove WorkaroundNetStandard target, change SDK for build from 3.0 preview to 3.1 stable version, fix error; change LockSDKVersion default value from empty string to false; apply Linux workaround in my change

Retarget to netcore5.0 (#3162)

* retarget to netcore5.0;  Linux script workaround need to apply for downloading SDK for build

add pkcs and cng packages

Temporary fix on patching SDK, add System.Security.Cryptography.Pkcs.dll and update deps.json, use dll from .nuget/packages

Address PR comments

implement add cert to store on Mac; display openssl version (#3166)

Implement timestamp integrity verification; Implement an equivalent NativeCms; enable signing APIs and tests (#3168)

* implement timestamp integrity verification; implement ManagedCms; enable signing APIs and tests

fix xplat verification branch after rebase (#3191)

Fixes: NuGet/Home#9012

check different cross-platform error messages for self-signed cert test (#3208)

Fixes: NuGet/Home#8933

Xplat Signing/Verification : Fix broken tests for setting privatekey of a X509Certificate2 (#3195)

use workaround to set private key for X509Certificate2 (netcore), change ToRSA to workaround for netcore(especially for non-windows)

enable PathTooLongException test (#3216)

Fixes: NuGet/Home#8920

enable TrustedSignerActionsProvider on netcore, along with tests (#3213)

Fixes: NuGet/Home#8921

Enable SignedPackageIntegrityVerificationTests for netcore5.0 (#3220)

Enable SignedPackageIntegrityVerificationTests for netcore, remove extra preprocessors

Enable timestamp provider test and fix 3 broken tests (#3210)

enable all unit tests; enable 8 more tests; enable timestampProviderTest; fix 3 broken tests in TimestampProviderTests

Fix 4 tests in SignatureTrustAndValidityVerificationProviderTests (#3242)

fix untrusted msg for different platforms

fixed broken tests using utility methods (#3263)

disable outdated tests (#3274)

enable 4 tests on Mac (#3275)

fix 5 broken tests in SignatureTrustAndValidityVerificationProviderTests, add workaround for Linux (#3252)

fix broken test case ExecuteCommandAsync_WithAmbiguousMatch_ThrowsAsync (#3291)

* Enable test ExecuteCommandAsync_WithMultiplePackagesAndInvalidCertificate_RaisesErrorsOnceAsync (#3342)

* enable a test, change the comments; extend test limitation on Linux

* Updated SDK version and removed the hardcoded version when reading from JSON file (#3372)

* update Pkcs package to the preview3 version (#3382)

* fix 2 warnings in CodeAnalysis (#3388)

* fix 2 warnings in CodeAnalysis

* disable dynamic revoke tests on Mac (#3373)

* fix 1 test (#3371)

* Fix more warnings reported by CodeAnalysis (#3393)

* fix string warnings raised by CodeAnalysis

* Retarget xplat verification to netcoreapp5.0 (#3386)

* retarget netcoreapp5.0 (a workaround for net5.0)

* fix all the rest warnings raised by code analysis (#3401)

* Suppress CA1307 warning for string.GetHashCode() method (#3404)

* Linux test failure due to generic exception (#3405)

* add TFM netcoreapp5.0 to N.B.T.Console and N.P.Core projects (#3419)

* add TFM netcoreapp5.0 to N.B.T.Console and N.P.Core projects

* add PIN prompt for netcore (#3389)

* Run dotnet nuget tests on multiple TFMs (#3387)

* enabled mac assertions

* Revert "enabled mac assertions"

This reverts commit d8a52e2.

* Enable test GetCertificateChain_WithUntrustedRoot_Throws on Mac (#3428)

* run signing integration tests cross plat (#3415)

* added GetNonce() to IRfc3161TimestampRequest interface (#3427)

* disable test for netcore (#3445)

* log Status and StatusInformation, fix 2 tests (#3268)

* Disable verification during extraction for Mono, disable a test on Mono (#3451)

* disable verification during extraction on mono,disable a mono test on Mac

* adapt 2 newly added dotnet integration tests for multiple TFMs (#3460)

* remove useless Sdk2x.targets

* get TimestampTests working again (#3218)

* get TimestampTests working again

Fixes: NuGet/Home#8935

* change GetTimestampAsync into internal

* fix runFuncTests.sh and InstallCLIforBuild.sh

* address feedback

* upgrade dotnet 5 to preview4 as preview5 is broken

* Fix exception thrown in SigningTestFixture during disposing (#3464)

Co-authored-by: Kartheek Penagamuri <52756182+kartheekp-ms@users.noreply.github.com>
Co-authored-by: Andy Zivkovic <zivkan@users.noreply.github.com>
Co-authored-by: Kat Marchán <kzm@zkat.tech>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants