Fix concurrency issues by removing file access #1

anthonylangsworth · 2017-03-04T12:32:47Z

I have replaced all XmlUrlResolver uses that reference files, usually using @tintoy 's create temp file methods, with XmlPreloadedResolver mapping the specified file(s) to the desired substitutions. This has many advantages:

There is no file access, hopefully resolving the concurrency issues.
This leaves the examples unchanged from the specs.
XmlPreloadedResolver throws an exception when a reference is unresolved whereas XmlUrlResolver returns an empty string. This is better behavior for tests.

I have cleaned up a few more tests while I was there. Most of the Transform tests appear to be copies of each other so there is scope to combine or parameterize these in the future.

CC: @krwq @tintoy @peterwurzinger

tintoy · 2017-03-04T13:00:55Z

src/System.Security.Cryptography.Xml/tests/TestHelpers.cs


 namespace System.Security.Cryptography.Xml.Tests
 {
+    public class TempFile : IDisposable


This already exists in the common test code:

https://github.com/tintoy/corefx/blob/feature/xml-crypto/test-files-base-class/src/System.Security.Cryptography.Xml/tests/System.Security.Cryptography.Xml.Tests.csproj#L49

tintoy · 2017-03-04T13:04:17Z

Nice one, @anthonylangsworth.

I wrote a quick-and-dirty XmlResolver that ignored everything but the filename component of the URI to deal with path issues, but your way works too ;-)

https://github.com/tintoy/corefx/blob/feature/xml-crypto/test-files-base-class/src/System.Security.Cryptography.Xml/tests/TestHelpers.cs#L62-L83
https://github.com/tintoy/corefx/blob/feature/xml-crypto/test-files-base-class/src/System.Security.Cryptography.Xml/tests/TestHelpers.cs#L16L29

anthonylangsworth · 2017-03-04T13:07:02Z

@tintoy I like what you have but I think a solution that bypasses file IO is preferred here. It is late so hit me with the clue bat if I have it wrong.

tintoy · 2017-03-04T13:14:51Z

What I linked to above was basically the same solution you had (custom XmlResolver, all in-memory, no file I/O), but ignoring the path component of the URI. It's all good - what we have now works, so I'm happy :)

tintoy · 2017-03-04T13:15:21Z

(ignore the other contents of that file - it was just a handy commit to point to)

anthonylangsworth · 2017-03-04T13:24:29Z

@tintoy Sorry. I must have misread the code.

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

src/System.Security.Cryptography.Xml/tests/TestHelpers.cs

+        /// <exception cref="XmlException">
+        /// <paramref name="inputXml"/> is not valid XML.
+        /// </exception>
+        public static string ExecuteTransform(string inputXml, Transform transform, Encoding encoding = null, XmlResolver resolver = null)


wow, this xml doc is long

Yeah, it's test code, but it's shared test code ;-)

Since this code is probably going to be shared across multiple test classes, I wanted to document it properly, including exceptions, so Intellisense will give the correct values.

krwq · 2017-03-04T19:25:27Z

src/System.Security.Cryptography.Xml/tests/XmlDsigC14NTransformTest.cs

        }

-        [Fact(Skip = "TODO: fix me")]
+        [Fact()]


nit: Fact()

krwq · 2017-03-04T19:27:10Z

Thanks @anthonylangsworth! Looks good - only comment about Fact() - we can fix it separately though - let's test this on corefx CI

Implement DsaKeyValue.LoadXml

Remove dead code from System.ComonentModel.TypeConverter for issue #1…

The following code throws an exception, caused by incorrect logic in BufferedStream.ReadByteSlow(): byte[] input = new byte[] { 1, 2 }; using (var reader = new BufferedStream(new MemoryStream(input), 2)) { reader.ReadByte(); reader.ReadByte(); reader.ReadByte(); byte[] mybuffer = new byte[10]; reader.Read(mybuffer, 0, 1); // throws System.ArgumentOutOfRangeException: 'Non-negative number required.' } When the input data is exhausted through a sequence of ReadByte() calls, ReadByteSlow() does not reset _readPos to zero. This causes a break in the contract specified in Read() - because because _readLen - _readPos is negative. Then the block copy fails because the count to copy is negative. Fix is to reset _readPos to zero before returning -1.

dotnet#26744) * Added tests for Microsoft.VisualBasic.CompilerServices.Conversions #14344 * added tests for ToBoolean fro Object

* NamedPipe: CurrentUserOnly, quick fixes for Unix * The path for the directory of when using current user only was wrong and not using the intended folder. * Added getpeerid validation on the server side. * Clean some dirty changes used for quick validation. * PR feedback round #1

* Fix ServiceController name population perf * Split tests * Remove dead field * Remove new use of DangerousGetHandle * SafeHandle all the things! * VSB #1 * VSB #2 * Fix GLE * Initialize machineName in ctor * Test for empty name ex * Null names * Inadvertent edit * Unix build * Move interop into class * Reverse SafeHandle for HAllocGlobal * Fix tests * Disable test for NETFX * CR feedback * Pattern matching on VSB * Direct call * typo

* Json prototype (#1) Basic API for Json writable DOM with scenarios including collection initialization, accessing and modifying Json nodes. * Json prototype - transformation API (#2) * transformation API added * assertions to existing scenarios added * Json prototype (#1) Basic API for Json writable DOM with scenarios including collection initialization, accessing and modifying Json nodes. * Json prototype - transformation API (#2) * transformation API added * assertions to existing scenarios added * JsonNumber implementation and tests (#3) * IEquatables added * JsonNumber implementation and unit tests added * pragmas deleted * review comments included, more tests added * decimal support added to JsonNumber * decimal support added to JsonArray and JsonObject * all unimplemented classes and methods with accompanying tests removed * First part of documentation added * documentation completed * missing exceptions added * JsonElement changes removed * part of the review comments included * work on review comments * code refactor * more decimal tests added using MemberData * more decimal tests added using MemberData * more test cases added * equals summary adjusted, equals tests added * more Equals tests added, GetHashCode tests added, minor changes * scientifing notation support added, rational numbers tests fixes * rational overflow tests added * ulong maxvalue tests added to rational types * presision problems fixes * exception strings fixed * CI failing fixes (hopefully), review comments included * missing == tests added to achieve 100% branch coverage * review comments included * trailing whitespaces fixes * equals comments added * equals object refactored to call quals json number * merge fix

anthonylangsworth added 7 commits March 4, 2017 21:45

Use XmlPreloadedResolver to remove file dependencies

bde5d64

Move String2Stream and ExecuteTransform into TestHelpers

c742452

Consolidate and fix up tests

8f8cb74

Use TestHelpers version of ExecuteTransform

088b6d5

Correct file:// reference to ensure examples are verbatim

9c2e262

Remove final TestHelpers.Create* reference

c66b24f

Remove TestHelpers.Create*

710775a

This was referenced Mar 4, 2017

[System.Security.Cryptography.Xml] Test plan for public classes tintoy/corefx#3

Open

Strategy for tests that use local files tintoy/corefx#9

Open

Fixing S.S.Cryptography.Xml tests dotnet/corefx#16681

Merged

tintoy reviewed Mar 4, 2017

View reviewed changes

krwq reviewed Mar 4, 2017

View reviewed changes

src/System.Security.Cryptography.Xml/tests/XmlDsigC14NTransformTest.cs

}

[Fact(Skip = "TODO: fix me")]

[Fact()]

Copy link

Owner

krwq Mar 4, 2017

Choose a reason for hiding this comment

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

nit: Fact()

krwq merged commit e5b43ed into krwq:signed-xml-fixing-tests-1 Mar 4, 2017

krwq pushed a commit that referenced this pull request Mar 6, 2017

Merge pull request #1 from tintoy/anthonylangsworth-monotests

d2e4569

Implement DsaKeyValue.LoadXml

krwq pushed a commit that referenced this pull request Jun 2, 2017

Merge pull request dotnet#20585 from norek/master

2ad4073

Remove dead code from System.ComonentModel.TypeConverter for issue #1…

krwq pushed a commit that referenced this pull request Mar 1, 2018

Added tests for Microsoft.VisualBasic.CompilerServices.Conversions #1… (

c705032

dotnet#26744) * Added tests for Microsoft.VisualBasic.CompilerServices.Conversions #14344 * added tests for ToBoolean fro Object

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix concurrency issues by removing file access #1

Fix concurrency issues by removing file access #1

Uh oh!

anthonylangsworth commented Mar 4, 2017

Uh oh!

tintoy Mar 4, 2017

Uh oh!

tintoy commented Mar 4, 2017

Uh oh!

anthonylangsworth commented Mar 4, 2017

Uh oh!

tintoy commented Mar 4, 2017

Uh oh!

tintoy commented Mar 4, 2017

Uh oh!

anthonylangsworth commented Mar 4, 2017

Uh oh!

krwq Mar 4, 2017

Uh oh!

tintoy Mar 4, 2017

Uh oh!

anthonylangsworth Mar 5, 2017

Uh oh!

krwq Mar 4, 2017

Uh oh!

krwq commented Mar 4, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Fix concurrency issues by removing file access #1

Fix concurrency issues by removing file access #1

Uh oh!

Conversation

anthonylangsworth commented Mar 4, 2017

Uh oh!

tintoy Mar 4, 2017

Choose a reason for hiding this comment

Uh oh!

tintoy commented Mar 4, 2017

Uh oh!

anthonylangsworth commented Mar 4, 2017

Uh oh!

tintoy commented Mar 4, 2017

Uh oh!

tintoy commented Mar 4, 2017

Uh oh!

anthonylangsworth commented Mar 4, 2017

Uh oh!

krwq Mar 4, 2017

Choose a reason for hiding this comment

Uh oh!

tintoy Mar 4, 2017

Choose a reason for hiding this comment

Uh oh!

anthonylangsworth Mar 5, 2017

Choose a reason for hiding this comment

Uh oh!

krwq Mar 4, 2017

Choose a reason for hiding this comment

Uh oh!

krwq commented Mar 4, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants