Fixing S.S.Cryptography.Xml tests by krwq · Pull Request #16681 · dotnet/corefx

krwq · 2017-03-03T19:24:00Z

cc @anthonylangsworth @tintoy @peterwurzinger

danmoseley · 2017-03-03T23:37:54Z

I asked the vendors to make a sweep over the weekend on corefx to change eg
[Fact(Skip = "https://github.com/dotnet/corefx/issues/16685")]
into

[Fact]
[ActiveIssue(16685)]

krwq · 2017-03-03T23:45:09Z

@danmosemsft I have intentionally not marked it as ActiveIssue becuase of https://github.com/dotnet/corefx/issues/16686 which is pretty annoying when working with VS

krwq · 2017-03-04T00:40:52Z

@tintoy @anthonylangsworth - I've just made a fix here for some of the yesteday's disabled test

danmoseley · 2017-03-04T00:46:34Z

Oh right..

krwq · 2017-03-04T00:54:24Z

or not - something is failing on non Windows - I'll take a look later

tintoy · 2017-03-04T00:54:45Z

Sorry @krwq - would love to help out but today's looking like a wash-out, I'm nowhere near my dev machine :-/

Happy to help out again tomorrow though!

(it's midday here)

krwq · 2017-03-04T00:57:04Z

It looks like just escaping issue. Not sure how do you properly escape entity references

System.Xml.XmlException : An XML comment cannot contain '--', and '-' cannot be the last character. Line 12, position 38.

The path seems to be containing three dashes in a row

tintoy · 2017-03-04T01:02:53Z

I think you can replace it with -...

tintoy · 2017-03-04T01:03:25Z

Sorry if I was at my dev machine I could try it out but I'm on a low-powered notebook at the moment.

krwq · 2017-03-04T01:11:56Z

wait - I'm not sure why I used 169 instead of 2D - probably copy paste fail

tintoy · 2017-03-04T01:26:55Z

Yeah I sometimes get it confused with en-dash (the long dash) but that's a bigger value I believe.

krwq · 2017-03-04T01:28:02Z

        System.Xml.XmlException : Fragment identifier '#2D;&#2D;&#2D;4526f5ff\bin\AnyOS.AnyCPU.Debug\System.Security.Cryptography.Xml.Tests\netcoreapp\XmlDsigExcC14NTransformTest.ExcC14NSpecExample5.txt' cannot be part of the system identifier 'D:\j\workspace\windows_nt_de&#2D;&#2D;&#2D;4526f5ff\bin\AnyOS.AnyCPU.Debug\System.Security.Cryptography.Xml.Tests\netcoreapp\XmlDsigExcC14NTransformTest.ExcC14NSpecExample5.txt'. Line 4, position 23.

we need some different way of escaping I guess or go to the solution which won't use files... I'm pretty tired of looking at those tiny issues today - I think I'll pass - if any of you would like to take a look please send a PR to my PR or separately. If you got time and will I think it might be a good idea to get a proper solution considering how much time we already spent for the workaround
cc: @anthonylangsworth @tintoy

tintoy · 2017-03-04T01:32:40Z

No worries - I may try expressing it as a file:// URI to see if that makes a difference.

krwq · 2017-03-04T19:28:48Z

@anthonylangsworth - you're right, I need to clean up my branches on my fork 😉 Thank you for help!

krwq · 2017-03-04T19:50:33Z

@anthonylangsworth still some tiny failures, likely some slash vs backslash issue considering it only failed on non windows although I haven't investigated.

Other option is that XmlPreloadedResolver might have an actual bug (if that's the case it's ok to make a fix but probably separate PR would be a better idea)

Fix Transform.PropagatedNamespace and test

krwq · 2017-03-06T07:26:54Z

@anthonylangsworth Thank you again!

…to signed-xml-fixing-tests-1

krwq · 2017-03-06T18:33:22Z

@anthonylangsworth I'll be taking over from where you've left now - Thanks again for another commit!

Fix for tests failing on Un*x due to "file:///"

krwq · 2017-03-06T19:11:53Z

@anthonylangsworth - Nice, everything green! Good job!!!

krwq · 2017-03-06T20:49:10Z

@anthonylangsworth @tintoy seems to me like dsaKeyValue.Key.ExportParameters(false); returns object whose Seed is null on linux - I'll investigate it

krwq · 2017-03-06T22:03:33Z

@anthonylangsworth @tintoy @peterwurzinger let's close on this PR and create a new one.

Here are the minimum items we need to re-enable the package:
https://github.com/dotnet/corefx/issues?q=is%3Aopen+is%3Aissue+milestone%3A2.0.0+label%3ASystem.Security.Cryptography.Xml+assignee%3Akrwq

This one requires writing new tests, anything else is bugs
https://github.com/dotnet/corefx/issues/16781

after that we can start improving the package.

We're almost there! Good job everyone! If you'd like to help on any of them please comment on the issue so we do not accidentally duplicate work.

bartonjs · 2017-03-06T21:59:13Z

        }

-        [Fact]
+        [Fact(Skip = "https://github.com/dotnet/corefx/issues/16779")]


[Fact] [ActiveIssue(16779)]

The commit description said the failure was Linux-only, but this test shows a Windows callstack... If it's actually non-Windows-only, then it'd be [ActiveIssue(16779, PlatformIDs.AnyUnix)]

@bartonjs I'm not converting to ActiveIssue while there is some active development happening and until this is fixed: https://github.com/dotnet/corefx/issues/16686 - in the issue I meant that the exactly the same test build and run against full .net framework is passing. possibly corefx test is failing when run on desktop or linux is using wrong assemblies - I'll investigate that when I get to this failure. I plan on fixing all of them before publishing the package.

bartonjs · 2017-03-06T22:01:00Z

                if (reference != null)
                    namespaces = reference._namespaces;
-                else if (signedXml._context != null)
+                else if (signedXml?._context != null)


netfx doesn't ?. here. Is this a netfx bug, or a test bug, that it needs to change?

@anthonylangsworth, do you remember why it needed to change?

@anthonylangsworth - I'm looking at the code and possibly this should be Context instead of _context. _context is being set only when someone specifically sets Context but in some cases Context is also set implicitly i.e. through setting Reference

If the tests found a product bug, but the product bug also exists in .NET Framework, we should leave the bug for now and open a new issue to track it (so that the fix gets marked port-consider and it's obvious what part we're port-considering).

@anthonylangsworth is this implementing this part correctly now? https://www.w3.org/TR/2001/REC-xml-c14n-20010315#NoNSPrefixRewriting - I'm not sure what PropagatedNamespaces should mean - I do not understand what does "namespace propagated into signature" is supposed to mean but likely it could mean what removed assert was checking: Assert.Equal(new StreamReader(s, Encoding.UTF8).ReadToEnd(), "<f:foo xmlns:f=\"urn:foo\"><b:bar xmlns:b=\"urn:bar\"></b:bar></f:foo>"); - was that implementing C14N-20000119 which is now not recommended? if yes, should we rather expose some new property to follow that so that we don't break existing code?

@bartonjs @krwq I can confirm netfx also throws a NullReferenceException on the second line below:

// Any class that inherits from Transform will do System.Security.Cryptography.Xml.XmlDsigC14NTransform transform = new XmlDsigC14NTransform(); System.Collections.Hashtable x = transform.PropagatedNamespaces;

I appear to have found and inadvertently fixed a framework bug. I'll revert this change later today. How do I raise the issue against .Net Framework?

@anthonylangsworth please keep the change, please file the issue in this repo, assign to me and point to your fix. ~~I'll take care of making sure this gets ported to full .NET~~ I'll do my best at least. I think the test should still be expecting prefixes in the elements though - was this also like that on netfx?

@krwq I'm pretty sure that our normal tactic here is to always have netfx-impacting changes be as small as possible; so this should stay broken for now, and get fixed as a separate issue.

@AlexGhiondea: Thoughts?

@krwq since this is a bug in Desktop, here is the process we should follow

Port the code as-is (don't introduce a change) -- this would bring the code to par with desktop

File a new issue for the change you are making

Tag the issue with netfx-port-consider this will flag it for consideration for Desktop

Follow the Desktop Shiproom process to get this change approved

If we start making fixes as we are porting the code there is a high chance that we are going to forget to port that change back to Desktop.

bartonjs · 2017-03-06T22:02:03Z

            s.Position = 0;

-            HashAlgorithm hash = HashAlgorithm.Create("System.Security.Cryptography.SHA1CryptoServiceProvider");
+            HashAlgorithm hash = SHA1.Create();


HashAlgorithm : IDisposable, this should be in a using.

bartonjs · 2017-03-06T22:05:30Z

            transform.LoadInnerXml(doc.ChildNodes);
-            Stream s = (Stream)transform.GetOutput();
-            Assert.Throws<ArgumentNullException>(() => Stream2Array(s));
+            Assert.Throws<ArgumentNullException>(() =>


What's supposed to ANE here? If it's checking that transform.GetOutput() is null then assert that it's null...

@bartonjs we are porting existing Mono tests right now and simply fixing errors happened during conversion from different test framework. I agree this is not ideal but I think we should do separate pass on these kind of clean ups

@krwq I appreciate that. But my point is that if you're making a change to the test you should make the right change. It seems weird to me to have a two line lambda expecting an ArgumentNullException. If the problem is that Mono and NetFx generate them from different places, add a comment. Otherwise, make the test test what we want it to test.... whatever that may be.

Converting to single line.

krwq · 2017-03-07T00:19:36Z

@anthonylangsworth I have disabled the test, left the change to the test and reverted the ?.. See https://github.com/dotnet/corefx/issues/16798

krwq · 2017-03-07T00:23:13Z

@bartonjs - I've changed SHA1 to be wrapped with using, changed ArgumentNullException test to have 1 line lambda and undid change introducing diff with Desktop

krwq · 2017-03-07T20:31:50Z

Thank you everyone for help here!

…sts-1 Fixing S.S.Cryptography.Xml tests Commit migrated from dotnet/corefx@718d569

dnfclas added the cla-already-signed label Mar 3, 2017

krwq force-pushed the signed-xml-fixing-tests-1 branch from c4c4521 to a6f0807 Compare March 3, 2017 19:25

karelz added the area-System.Security label Mar 3, 2017

karelz assigned krwq Mar 3, 2017

karelz added the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Mar 3, 2017

krwq added 9 commits March 3, 2017 16:15

fix tests trying to create SHA1CryptoServiceProvider

c435f41

disable MAC3DES test, add DSASignatureDescription

32ab581

fix or better reason for test failure

fd114f9

disable AsymmetricDSASignature test with different reason

ff68844

fix improperly wrapped Assert.Throws (verified with Mono)

8de1a2a

fix reverse condition

41864db

disable few more tests with better issue

d5f7335

expect exception on unsupported output

efb7150

fix few more tests

5b4dddf

krwq force-pushed the signed-xml-fixing-tests-1 branch from 81953be to 5b4dddf Compare March 4, 2017 00:40

attempt to fix escaping issue

4ed2428

fix replacement char

a556ced

anthonylangsworth and others added 3 commits March 5, 2017 21:26

Fix PropagatedNamespaces and test

258206f

[Fact()] to [Fact]

42612de

Merge pull request #2 from anthonylangsworth/PropagatedNamespace

2740de0

Fix Transform.PropagatedNamespace and test

anthonylangsworth added 2 commits March 6, 2017 23:10

Merge branch 'signed-xml-fixing-tests-1' of github.com:krwq/corefx in…

95cb211

…to signed-xml-fixing-tests-1

Fix slash error in Un*x

fe36785

Merge pull request #3 from anthonylangsworth/slashfix

34aff2c

Fix for tests failing on Un*x due to "file:///"

assign more concrete issues to disabled tests

b079fd0

krwq changed the title ~~[WIP][NoMerge] Fixing S.S.Cryptography.Xml tests~~ Fixing S.S.Cryptography.Xml tests Mar 6, 2017

krwq removed the * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) label Mar 6, 2017

krwq requested review from bartonjs and morganbr March 6, 2017 19:55

disable test failing on linux

2a5a8eb

bartonjs suggested changes Mar 6, 2017

View reviewed changes

krwq added 2 commits March 6, 2017 15:54

apply review feedback

caf7de2

revert product fix and disable test

4b01e4b

bartonjs approved these changes Mar 7, 2017

View reviewed changes

krwq merged commit 718d569 into dotnet:master Mar 7, 2017

karelz modified the milestone: 2.0.0 Mar 10, 2017

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

Merge pull request dotnet/corefx#16681 from krwq/signed-xml-fixing-te…

f6fa7f9

…sts-1 Fixing S.S.Cryptography.Xml tests Commit migrated from dotnet/corefx@718d569

Conversation

krwq commented Mar 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danmoseley commented Mar 3, 2017

Uh oh!

krwq commented Mar 3, 2017

Uh oh!

krwq commented Mar 4, 2017

Uh oh!

danmoseley commented Mar 4, 2017

Uh oh!

krwq commented Mar 4, 2017

Uh oh!

tintoy commented Mar 4, 2017

Uh oh!

krwq commented Mar 4, 2017

Uh oh!

tintoy commented Mar 4, 2017

Uh oh!

tintoy commented Mar 4, 2017

Uh oh!

krwq commented Mar 4, 2017

Uh oh!

tintoy commented Mar 4, 2017

Uh oh!

krwq commented Mar 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tintoy commented Mar 4, 2017

Uh oh!

krwq commented Mar 4, 2017

Uh oh!

krwq commented Mar 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

krwq commented Mar 6, 2017

Uh oh!

krwq commented Mar 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

krwq commented Mar 6, 2017

Uh oh!

krwq commented Mar 6, 2017

Uh oh!

krwq commented Mar 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krwq Mar 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krwq Mar 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anthonylangsworth Mar 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

krwq Mar 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

krwq commented Mar 3, 2017 •

edited

Loading

krwq commented Mar 4, 2017 •

edited

Loading

krwq commented Mar 4, 2017 •

edited

Loading

krwq commented Mar 6, 2017 •

edited

Loading

krwq Mar 6, 2017 •

edited

Loading

krwq Mar 6, 2017 •

edited

Loading

anthonylangsworth Mar 6, 2017 •

edited

Loading

krwq Mar 6, 2017 •

edited

Loading

krwq commented Mar 7, 2017 •

edited

Loading