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

fix broken func test in SignatureUtilityTests #3245

Conversation

kartheekp-ms
Copy link
Contributor

@kartheekp-ms kartheekp-ms commented Feb 19, 2020

Bug

Fixes: NuGet/Home#8937
Regression: No

Fix

Details: A func test to generate a signed package with no certificate usage is working in only net472 platform but fails in all the netcore platforms. As per my conversation with @dtivel, I came to know that package signing libraries for net472 platform were built by NuGet team where as for netcore platforms we rely on dotnet libraires. It looks like modifying net472 NuGet libraries to fix this func test is not a good long-term solution. Hence submitting this PR to split the failing func test into 2 where one test generates a signed package with no certificate usage on net472 platform and other test verifies the package content across all the platforms. @heng-liu - thank you for developing the new build infrastructure where signed packages generated by various func tests are uploaded to a container and downloaded later for verification purposes.

Testing/Validation

Tests Added: Yes

@kartheekp-ms kartheekp-ms changed the title Dev kartheekp ms xplat tests 8937 fix broken func test in SignatureUtilityTests Feb 19, 2020
test/TestUtilities/Test.Utility/Signing/Constants.cs Outdated Show resolved Hide resolved
test/TestUtilities/Test.Utility/Signing/Constants.cs Outdated Show resolved Hide resolved
test/TestUtilities/Test.Utility/Signing/Constants.cs Outdated Show resolved Hide resolved
public async Task VerifySignaturesAsync_PreGenerateSignedPackages_AuthorSigned_TimeStampedWithNoSigningCertificateUsage_Throws(string dir)
{
// Arrange
var caseName = "ATNOCERTIFICATEUSAGE";
Copy link
Contributor

Choose a reason for hiding this comment

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

A couple problems:

  • This test naming scheme is cryptic. It's not clear how this was generated, what it means, or how I would generate a new name successfully.
  • caseName is confusing; don't we already have a test case name with VerifySignaturesAsync_PreGenerateSignedPackages_AuthorSigned_TimeStampedWithNoSigningCertificateUsage_Throws?
  • Two files are expected to have the same string literal for everything to work successfully.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Discussed few possible options with @dtivel

  • Build case names dynamically using a builder design pattern. The problem with this approach is it will be hard to search for case names during design/compile time.
  • Use string constants for case names but we can't avoid duplicates values (if created unintentionally).

@dtivel suggested to use enum that can help solve above drawabacks.

@kartheekp-ms kartheekp-ms requested a review from dtivel February 21, 2020 15:29
@@ -126,6 +126,10 @@
<data name="Argument_Must_Be_GreaterThanOrEqualTo" xml:space="preserve">
<value>Value must be greater than or equal to {0}</value>
</data>
<data name="Error_DirectoryDoesNotExist" xml:space="preserve">
<value>Directory {0} does not exist.</value>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add single quotes, or similar, around the {0}, to better handle the long name, with potential spaces? What do we usually do?

if (!Directory.Exists(path))
{
Directory.CreateDirectory(path);
}

//Create a folder for a each platform, under PreGenPackages folder.
//Create a folder for a each platform, under GeneratedPackages folder.
//For functional test on windows, 2 folders will be created.
Copy link
Contributor

Choose a reason for hiding this comment

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

is this comment about "2 folders" will be created still true. it isn't clear to my why that is true. should the comment be enhanced?

/// </summary>
public enum FolderNames
{
One,
Copy link
Contributor

Choose a reason for hiding this comment

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

these enum values didn't give me a clue about the purpose...the summary helps. Perhaps better enum name or value names could make it more obvious??

Copy link
Contributor

Choose a reason for hiding this comment

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

How about:

public enum TestPackages
{
    Package1
}

You could be fancier than Package1 and do what you did before and embed more scenario information in the name. I would avoid trying to encode everything in the name (e.g.: "ATNOCERTIFICATE" for "Author signed with Timestamp with No Certificate in the timestamp signature").

My issues with the previous approach were:

  • It's not easy to understand the string. ("A" and "T" are not common abbreviations and "NOCERTIFICATE" is vague about which certificate and how it is missing.)
  • It's not easy to enforce uniqueness in string values. (It's possible that another test could use the same string and overwrite/use another test package.)
  • There are rules around these string literals which are not easily enforced. (The same string literal needs to be used on both the package generation side and the package consumption side.)

An enum makes a good alternative and the member names don't matter too much. You could have an XML doc comment on the member which describes the package requirements in detail:

public enum TestPackages
{
    /// <summary>
    /// This package is author signed with a timestamp.
    /// The timestamp signature does not include the signing certificate.
    /// Certificates are otherwise trusted and valid.
    /// </summary>
    Package1
}

public static class TestFolderNames
{
public const string Windows_NetFullFrameworkFolder = "Windows_NetFullFramework";
public const string Windows_NetCoreFolder = "Windows_NetCore";
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: netcore is about to be ".net 5"...
in a few years, netcore will be an old fashioned term.

Copy link
Contributor

Choose a reason for hiding this comment

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

how should we be distinguishing between legacy framework and .net "core" going forward, then?

src/NuGet.Core/NuGet.Common/PathUtil/FileUtility.cs Outdated Show resolved Hide resolved
src/NuGet.Core/NuGet.Common/PathUtil/FileUtility.cs Outdated Show resolved Hide resolved
/// <summary>
/// Folder names ensure uniqueness in path when signed packages are stored on disk for verification in later steps
/// </summary>
public enum FolderNames
Copy link
Contributor

Choose a reason for hiding this comment

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

One type per file, so FolderNames in FolderNames.cs and TestFolderNames in TestFolderNames.cs

/// </summary>
public enum FolderNames
{
One,
Copy link
Contributor

Choose a reason for hiding this comment

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

How about:

public enum TestPackages
{
    Package1
}

You could be fancier than Package1 and do what you did before and embed more scenario information in the name. I would avoid trying to encode everything in the name (e.g.: "ATNOCERTIFICATE" for "Author signed with Timestamp with No Certificate in the timestamp signature").

My issues with the previous approach were:

  • It's not easy to understand the string. ("A" and "T" are not common abbreviations and "NOCERTIFICATE" is vague about which certificate and how it is missing.)
  • It's not easy to enforce uniqueness in string values. (It's possible that another test could use the same string and overwrite/use another test package.)
  • There are rules around these string literals which are not easily enforced. (The same string literal needs to be used on both the package generation side and the package consumption side.)

An enum makes a good alternative and the member names don't matter too much. You could have an XML doc comment on the member which describes the package requirements in detail:

public enum TestPackages
{
    /// <summary>
    /// This package is author signed with a timestamp.
    /// The timestamp signature does not include the signing certificate.
    /// Certificates are otherwise trusted and valid.
    /// </summary>
    Package1
}

@kartheekp-ms kartheekp-ms force-pushed the dev-kartheekp-ms-xplat-tests-8937 branch 2 times, most recently from 4ccbdeb to e9e6262 Compare March 3, 2020 21:10
@zkat zkat force-pushed the dev-zkat-cross-verification-tests branch from 41bb593 to cd7e2e1 Compare March 4, 2020 19:46
@zkat zkat force-pushed the dev-zkat-cross-verification-tests branch from cd7e2e1 to aaf2c39 Compare March 5, 2020 18:38
@kartheekp-ms kartheekp-ms force-pushed the dev-kartheekp-ms-xplat-tests-8937 branch from cc29df2 to ee79ceb Compare March 5, 2020 21:44
@kartheekp-ms kartheekp-ms changed the base branch from dev-zkat-cross-verification-tests to dev-feature-xplatVerification March 5, 2020 21:44
@zkat zkat changed the base branch from dev-feature-xplatVerification to dev-zkat-cross-verification-tests March 9, 2020 17:20
@kartheekp-ms kartheekp-ms changed the base branch from dev-zkat-cross-verification-tests to dev-feature-xplatVerification March 9, 2020 17:20
@zkat zkat changed the base branch from dev-feature-xplatVerification to dev-zkat-cross-verification-tests March 9, 2020 17:20
@kartheekp-ms kartheekp-ms force-pushed the dev-kartheekp-ms-xplat-tests-8937 branch from ee79ceb to 42fc69c Compare March 9, 2020 17:33
@kartheekp-ms kartheekp-ms merged commit b66d421 into dev-zkat-cross-verification-tests Mar 9, 2020
@zkat zkat deleted the dev-kartheekp-ms-xplat-tests-8937 branch March 12, 2020 18:06
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