Add tests for WritableBuffer extensions and rename WriteAscii => WriteAsciiNoValidation #1553

natemcmaster · 2017-03-24T20:16:44Z

Add tests for WritableBuffer.{WriteNumeric, WriteAscii}
Rename WriteAsciiNoValidation to make it clearer that it does not perform ascii validation.

Also, this changes the xunit display settings to make it easier to view tests in Test Explorer. (cref microsoft/vstest#623 )

Resolves #1508

benaadams · 2017-03-24T20:24:34Z

Also, this changes the xunit display settings to make it easier to view the write test in Test Explorer

TFFT 👍

cesarblum · 2017-03-24T23:06:27Z

...re.Server.Kestrel.FunctionalTests/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests.csproj

@@ -37,6 +37,12 @@
    <Service Include="{82a7f48d-3b50-4b1e-b82e-3ada8210c358}" />
  </ItemGroup>

+  <ItemGroup>
+    <None Update="xunit.runner.json">
+      <CopyToOutputDirectory>PreserveNewest</CopyToOutputDirectory>


cesarblum · 2017-03-24T23:07:09Z

test/Microsoft.AspNetCore.Server.Kestrel.FunctionalTests/xunit.runner.json

@@ -0,0 +1,5 @@
+{
+  "$schema": "http://json.schemastore.org/xunit.runner.schema",
+  "methodDisplay": "method",


Is there a way to display as Class.Method?

I wish. There are only two options. "classAndMethod" and "method". The former is the default and is what results in the massively long test names in Test Explorer.

cesarblum · 2017-03-24T23:07:44Z

test/Microsoft.AspNetCore.Server.KestrelTests/PipelineExtensionTests.cs

+{
+    public class PipelineExtensionTests : IDisposable
+    {
+        // ulong.MaxValue.ToString().Length


Make it readonly and call that here.

Because it is used in an attribute it has to be a const.

cesarblum · 2017-03-24T23:08:25Z

test/Microsoft.AspNetCore.Server.KestrelTests/PipelineExtensionTests.cs

+        // ulong.MaxValue.ToString().Length
+        private const int _ulongMaxValueLength = 20; 
+
+        private readonly IPipe _pipe;


Why shared state in tests? 😢

xUnit.net creates a new instance of the test class for every test that is run

Ok. I'm still sad but I can accept this. I think.

I like tests to be self-contained. They can call helper methods and such, but generally I avoid having class state like the plague. Ideally every test should be a self-contained mini spec of an expected behavior of the system.

cesarblum · 2017-03-24T23:08:57Z

test/Microsoft.AspNetCore.Server.KestrelTests/PipelineExtensionTests.cs

+        [Theory]
+        [InlineData(ulong.MinValue)]
+        [InlineData(ulong.MaxValue)]
+        [InlineData(4_8_15_16_23_42)]


I wanted to test a random number somewhere between 0 and ulong.MaxValue. http://lost.wikia.com/wiki/The_Numbers

Ah, I'm not familiar with Lost references.

cesarblum · 2017-03-24T23:12:59Z

test/Microsoft.AspNetCore.Server.KestrelTests/PipelineExtensionTests.cs

+            Assert.Equal(input.Length, reader.Buffer.Length);
+        }
+
+        private async Task<ReadResult> WriteAscii(string input)


Call it WriteAsciiNoValidation too, otherwise it's a little confusing to read.

cesarblum · 2017-03-24T23:13:07Z

test/Microsoft.AspNetCore.Server.KestrelTests/PipelineExtensionTests.cs

+        [InlineData("𤭢𐐝")]
+        // non-ascii characters stored in 16 bits
+        [InlineData("ñ٢⛄⛵")]
+        public async Task WriteAsciiWritesOnlyOneBytePerChar(string input)


WriteAsciiNoValidationWritesOnlyOneBytePerChar

cesarblum · 2017-03-24T23:14:36Z

test/Microsoft.AspNetCore.Server.KestrelTests/PipelineExtensionTests.cs

+        }
+
+        [Theory]
+        [InlineData(ulong.MinValue)]


Add case for single digit.

It's ulong not long like my brain parsed so ignore this.

cesarblum · 2017-03-24T23:15:55Z

test/Microsoft.AspNetCore.Server.KestrelTests/PipelineExtensionTests.cs

+
+namespace Microsoft.AspNetCore.Server.KestrelTests
+{
+    public class PipelineExtensionTests : IDisposable


Add a WriteAsciiNoValidation test encoding and verifying a single char, with data from 0-255. This guarantees no character value is special-cased in that method.

Edit: wording was super broken.

I added a test for everything in the ASCII range, 0x00 - 0x7F. As mentioned here #1508 (comment), characters outside the ASCII range should never reach this point...in theory.

cesarblum · 2017-03-24T23:16:30Z

test/Microsoft.AspNetCore.Server.KestrelTests/PipelineExtensionTests.cs

+        [InlineData("!", new byte[] { 33 })]
+        // null or empty
+        [InlineData("", new byte[0])]
+        [InlineData(default(string), new byte[0])]


Explicit null.

cesarblum · 2017-03-27T17:35:26Z

test/Microsoft.AspNetCore.Server.KestrelTests/PipelineExtensionTests.cs

+        {
+            const byte maxAscii = 0x7f;
+            var writer = _pipe.Writer.Alloc();
+            for (var i = 0; i < maxAscii; i++)


Use MemberData instead of looping. Easier to figure out what's wrong if this ever breaks.

Would it kill vstest performance even more?

Seems unnecessary to make it a theory. The test only has one assert. So if it goes wrong, the assert error would be

Assert.Equal() Failure Expected: (x) Actual: (y)

Sounds good.

cesarblum · 2017-03-27T17:36:03Z

test/Microsoft.AspNetCore.Server.KestrelTests/PipelineExtensionTests.cs

+            AssertExtensions.Equal(Encoding.ASCII.GetBytes(testString), written.ToArray());
+        }
+
+        public void Dispose()


nit: move this to the top

cesarblum · 2017-03-27T17:37:09Z

test/Microsoft.AspNetCore.Server.KestrelTests/TestHelpers/AssertExtensions.cs

+            {
+                if (expected[i] != actual[i])
+                {
+                    throw new XunitException($@"Expected byte at index {i} to be '\x{expected[i]}' but was '\x{actual[i]}'");


Bytes will be printed in decimal. Seems like you wanted hex though.

cesarblum

when green.

natemcmaster · 2017-03-27T18:53:52Z

@halter73 ok if I merge, or should I wait for your transport refactor #1551?

…eAsciiNoValidation

natemcmaster · 2017-03-29T23:14:49Z

Rebased. ⌚ CI

natemcmaster requested review from davidfowl and pakrym March 24, 2017 20:16

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

cesarblum suggested changes Mar 24, 2017

View reviewed changes

cesarblum suggested changes Mar 27, 2017

View reviewed changes

cesarblum approved these changes Mar 27, 2017

View reviewed changes

natemcmaster added blocked Blocked and removed blocked Blocked labels Mar 29, 2017

Add tests for WritableBuffer extensions and rename WriteAscii => Writ…

686829d

…eAsciiNoValidation

natemcmaster force-pushed the namc/buffer-ext-test branch from a8d67d1 to 686829d Compare March 29, 2017 23:14

natemcmaster merged commit 686829d into dev Mar 29, 2017

natemcmaster deleted the namc/buffer-ext-test branch March 29, 2017 23:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tests for WritableBuffer extensions and rename WriteAscii => WriteAsciiNoValidation #1553

Add tests for WritableBuffer extensions and rename WriteAscii => WriteAsciiNoValidation #1553

natemcmaster commented Mar 24, 2017 •

edited

Loading

benaadams commented Mar 24, 2017

cesarblum Mar 24, 2017

cesarblum Mar 24, 2017

natemcmaster Mar 24, 2017

cesarblum Mar 27, 2017

cesarblum Mar 24, 2017

natemcmaster Mar 27, 2017

cesarblum Mar 24, 2017

natemcmaster Mar 24, 2017

cesarblum Mar 24, 2017

cesarblum Mar 24, 2017

natemcmaster Mar 24, 2017

cesarblum Mar 24, 2017

davidfowl Mar 25, 2017

cesarblum Mar 24, 2017

cesarblum Mar 24, 2017

cesarblum Mar 24, 2017

natemcmaster Mar 24, 2017

cesarblum Mar 24, 2017 •

edited

Loading

cesarblum Mar 24, 2017 •

edited

Loading

natemcmaster Mar 27, 2017

cesarblum Mar 24, 2017

cesarblum Mar 27, 2017

pakrym Mar 27, 2017

natemcmaster Mar 27, 2017

cesarblum Mar 27, 2017

cesarblum Mar 27, 2017

cesarblum Mar 27, 2017

cesarblum left a comment •

edited

Loading

natemcmaster commented Mar 27, 2017

natemcmaster commented Mar 29, 2017

Add tests for WritableBuffer extensions and rename WriteAscii => WriteAsciiNoValidation #1553

Add tests for WritableBuffer extensions and rename WriteAscii => WriteAsciiNoValidation #1553

Conversation

natemcmaster commented Mar 24, 2017 • edited Loading

benaadams commented Mar 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cesarblum Mar 24, 2017 • edited Loading

Choose a reason for hiding this comment

cesarblum Mar 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cesarblum left a comment • edited Loading

Choose a reason for hiding this comment

natemcmaster commented Mar 27, 2017

natemcmaster commented Mar 29, 2017

natemcmaster commented Mar 24, 2017 •

edited

Loading

cesarblum Mar 24, 2017 •

edited

Loading

cesarblum Mar 24, 2017 •

edited

Loading

cesarblum left a comment •

edited

Loading