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.

Sign up for GitHub

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

Jump to bottom

get TimestampTests working again #3218

Merged
merged 4 commits into from
Jun 18, 2020
Merged

get TimestampTests working again #3218

merged 4 commits into from
Jun 18, 2020

Conversation

zkat
Copy link
Contributor

@zkat zkat commented Jan 30, 2020 •
edited
Loading

@zkat zkat force-pushed the dev-zkat-fix-timestamp-tests branch from 412bd21 to 6178ba7 Compare February 4, 2020 23:09
@zkat zkat marked this pull request as ready for review February 4, 2020 23:10
@zkat zkat force-pushed the dev-zkat-fix-timestamp-tests branch from 6178ba7 to 5a28a21 Compare February 4, 2020 23:46
@zkat zkat requested a review from dtivel February 4, 2020 23:50
@zkat zkat force-pushed the dev-zkat-fix-timestamp-tests branch from 5a28a21 to a44824d Compare February 5, 2020 00:17
kartheekp-ms
kartheekp-ms reviewed Feb 7, 2020
View reviewed changes
test/TestUtilities/Test.Utility/Signing/SigningTestUtility.cs Outdated
AssertOfflineRevocation(issues, logLevel, NuGetLogCode.NU3018);
}

public static void AssertOfflineRevocation(IEnumerable<ILogMessage> issues, LogLevel logLevel, NuGetLogCode code)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think these 2 methods can be refactored to a single method because the only difference is the message. May be we can add additional parameter whose value is checked against the issues collection (or) invoke Assert.All

public static void AssertOfflineRevocation
public static void AssertRevocationStatusUnknown

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review! I disagree about refactoring, since the methods are already fairly minimal on their own. I think refactoring them further would be a bit much, no? The bulk of their bodies is, after all, just picking that message, as you say.

@zkat zkat force-pushed the dev-zkat-fix-timestamp-tests branch from a44824d to ab13f09 Compare February 14, 2020 00:25
@zkat zkat requested a review from heng-liu February 14, 2020 00:57
@zkat zkat force-pushed the dev-zkat-fix-timestamp-tests branch from ab13f09 to fe75082 Compare February 14, 2020 22:25
@zkat zkat force-pushed the dev-feature-xplatVerification branch 4 times, most recently from 4b62d51 to bc243eb Compare March 20, 2020 23:56
@heng-liu heng-liu force-pushed the dev-feature-xplatVerification branch from bc243eb to e94f122 Compare March 27, 2020 21:22
@heng-liu heng-liu force-pushed the dev-feature-xplatVerification branch from 0bc5fb5 to 6aae258 Compare May 7, 2020 17:00
@nkolev92
Copy link
Member

nkolev92 commented May 8, 2020

@zkat does this need rebased on top of the latest feature branch?

@zkat
Copy link
Contributor Author

zkat commented May 8, 2020

It does!

@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
@zkat zkat force-pushed the dev-zkat-fix-timestamp-tests branch from fe75082 to a92151e Compare June 15, 2020 18:12
@zkat zkat requested a review from aortiz-msft as a code owner June 15, 2020 18:12
@heng-liu heng-liu force-pushed the dev-zkat-fix-timestamp-tests branch from a92151e to 0d78b1a Compare June 16, 2020 20:49
@heng-liu heng-liu force-pushed the dev-feature-xplatVerification branch from 91d697b to d5a3abd Compare June 17, 2020 01:25
@heng-liu heng-liu force-pushed the dev-zkat-fix-timestamp-tests branch from 0d78b1a to c4d5c53 Compare June 17, 2020 01:50
dominoFire
dominoFire reviewed Jun 17, 2020
View reviewed changes
Copy link
Contributor

@dominoFire dominoFire left a comment

Choose a reason for hiding this comment

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

Some comments on string constants in tests

test/TestUtilities/Test.Utility/Signing/SigningTestUtility.cs Outdated Show resolved Hide resolved
test/TestUtilities/Test.Utility/Signing/SigningTestUtility.cs Outdated Show resolved Hide resolved
@heng-liu heng-liu requested a review from dominoFire June 17, 2020 20:59
@heng-liu heng-liu merged commit 418ed92 into dev-feature-xplatVerification Jun 18, 2020
@heng-liu heng-liu deleted the dev-zkat-fix-timestamp-tests branch June 18, 2020 15:12
heng-liu pushed a commit that referenced this pull request Jun 19, 2020
@zkat @heng-liu
get TimestampTests working again (#3218)
0e7fadc 
* get TimestampTests working again

Fixes: NuGet/Home#8935
heng-liu pushed a commit that referenced this pull request Jun 19, 2020
@zkat @heng-liu
get TimestampTests working again (#3218)
b1342aa 
* get TimestampTests working again

Fixes: NuGet/Home#8935
aortiz-msft pushed a commit that referenced this pull request Jun 20, 2020
@heng-liu @kartheekp-ms
@zivkan @zkat
Enable cross-platform package signature verification for restore (#3458)
784274c 
* 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
Reviewers

@kartheekp-ms kartheekp-ms kartheekp-ms left review comments

@zivkan zivkan zivkan approved these changes

@dominoFire dominoFire dominoFire approved these changes

@donnie-msft donnie-msft Awaiting requested review from donnie-msft

@nkolev92 nkolev92 Awaiting requested review from nkolev92

@dtivel dtivel Awaiting requested review from dtivel

@heng-liu heng-liu Awaiting requested review from heng-liu

@aortiz-msft aortiz-msft Awaiting requested review from aortiz-msft

@cristinamanum cristinamanum Awaiting requested review from cristinamanum

@rrelyea rrelyea Awaiting requested review from rrelyea

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@zkat @nkolev92 @dominoFire @zivkan @dtivel @heng-liu @kartheekp-ms