Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Disable Aes test TestDecryptorReusability when quirk disabled or old netfx #18945

Merged
merged 3 commits into from
Apr 27, 2017

Conversation

krwq
Copy link
Member

@krwq krwq commented Apr 25, 2017

Fixes: https://github.com/dotnet/corefx/issues/18903

I've manually tested the test is still being run on Core

@@ -13,6 +13,10 @@ public static class DecryptorReusabilty
[Fact]
public static void TestDecryptorReusability()
{
// See https://github.com/dotnet/corefx/issues/18903 for details
if (!ShouldDescriptorBeReusable())
return;
Copy link
Member

Choose a reason for hiding this comment

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

We should do this with a ConditionalFact instead, e.g.

[ConditionalFact(nameof(ShouldDescriptorBeReusable))]

That way it'll show as a skipped test in the output if the check fails.

Copy link
Member Author

Choose a reason for hiding this comment

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

Didn't know about this - thanks @stephentoub !

@krwq
Copy link
Member Author

krwq commented Apr 26, 2017

@bartonjs PTAL

@@ -10,7 +10,8 @@ namespace System.Security.Cryptography.Encryption.Aes.Tests

public static class DecryptorReusabilty
{
[Fact]
// See https://github.com/dotnet/corefx/issues/18903 for details
[ConditionalFact(nameof(ShouldDescriptorBeReusable))]
Copy link
Member Author

Choose a reason for hiding this comment

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

Decryptor not Descriptor 🤦‍♂️

Copy link
Member

@bartonjs bartonjs left a comment

Choose a reason for hiding this comment

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

Approved, please fix your self-identified typo :)

@krwq krwq merged commit 84bb57b into dotnet:master Apr 27, 2017
@karelz karelz modified the milestone: 2.0.0 Apr 28, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants