Fixes to re-enable Pipeline tests that were skipped. #1432
Conversation
| { | ||
| try | ||
| { | ||
| var tuple = await PingClient(connection, SendCount); | ||
| var tuple = PingClient(connection, SendCount).GetResultSync(); |
There was a problem hiding this comment.
Instead of doing these changes, does it make sense to just change PingClient and keep the test as async Task?
https://github.com/ahsonkhan/corefxlab/blob/FixingTests/tests/System.IO.Pipelines.Tests/SocketsFacts.cs#L185
There was a problem hiding this comment.
What is using span in this test?
There was a problem hiding this comment.
Nothing. Your change here fixed the issue: #1421
| [Fact(Skip = "System.TypeLoadException : A value type containing a by-ref instance field, such as Span<T>, cannot be used as the type for a class instance field.")] | ||
| public async Task ReadTWorksAgainstSimpleBuffers() | ||
| [Fact] | ||
| public void ReadTWorksAgainstSimpleBuffers() |
There was a problem hiding this comment.
[Fact]
public void ReadTWorksAgainstSimpleBuffers()
{
var readable = BufferUtilities.CreateBuffer(new byte[] { 0, 1, 2, 3, 4, 5, 6, 7 });
var span = readable.ToSpan();
Assert.True(readable.IsSingleSpan);
Assert.Equal(span.Read<byte>(), readable.ReadLittleEndian<byte>());
Assert.Equal(span.Read<sbyte>(), readable.ReadLittleEndian<sbyte>());
Assert.Equal(span.Read<short>(), readable.ReadLittleEndian<short>());
Assert.Equal(span.Read<ushort>(), readable.ReadLittleEndian<ushort>());
Assert.Equal(span.Read<int>(), readable.ReadLittleEndian<int>());
Assert.Equal(span.Read<uint>(), readable.ReadLittleEndian<uint>());
Assert.Equal(span.Read<long>(), readable.ReadLittleEndian<long>());
Assert.Equal(span.Read<ulong>(), readable.ReadLittleEndian<ulong>());
Assert.Equal(span.Read<float>(), readable.ReadLittleEndian<float>());
Assert.Equal(span.Read<double>(), readable.ReadLittleEndian<double>());
}
[Fact]
public void ReadTWorksAgainstMultipleBuffers()
{
var readable = BufferUtilities.CreateBuffer(new byte[] { 0, 1, 2 }, new byte[] { 3, 4, 5 }, new byte[] { 6, 7, 9 });
Assert.Equal(9, readable.Length);
int spanCount = 0;
foreach (var _ in readable)
{
spanCount++;
}
Assert.Equal(3, spanCount);
var span = readable.ToSpan();
Assert.Equal(span.Read<byte>(), readable.ReadLittleEndian<byte>());
Assert.Equal(span.Read<sbyte>(), readable.ReadLittleEndian<sbyte>());
Assert.Equal(span.Read<short>(), readable.ReadLittleEndian<short>());
Assert.Equal(span.Read<ushort>(), readable.ReadLittleEndian<ushort>());
Assert.Equal(span.Read<int>(), readable.ReadLittleEndian<int>());
Assert.Equal(span.Read<uint>(), readable.ReadLittleEndian<uint>());
Assert.Equal(span.Read<long>(), readable.ReadLittleEndian<long>());
Assert.Equal(span.Read<ulong>(), readable.ReadLittleEndian<ulong>());
Assert.Equal(span.Read<float>(), readable.ReadLittleEndian<float>());
Assert.Equal(span.Read<double>(), readable.ReadLittleEndian<double>());
}
| EnsureSpanDisposed(preserved.Buffer.First); | ||
| } | ||
|
|
||
| [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.NoOptimization)] |
There was a problem hiding this comment.
Are these attributes necessary?
There was a problem hiding this comment.
I was ensuring these weren't optimized away since nothing in that code is ever used.
There was a problem hiding this comment.
I still don't understand why it is necessary.
If the method call can get optimized out, won't this get optimized too:
https://github.com/dotnet/corefxlab/pull/1432/files#diff-920b95938a7bd6c56992737a476f216eL326
// Make sure we can acccess the span
var span = buffer.First.Span;There was a problem hiding this comment.
It could in release builds, if the compiler can determine that accessing First and Span are side-effect free.
|
LGTM |
|
@shiftylogic, is something broken? It seems like the tests never end: |
|
@dotnet-bot test this please |
|
Build timed out (after 120 minutes). Marking the build as aborted. |
|
@ahsonkhan Hmm interesting. I made a similar change in another PR and the build timed out in debug. |
|
@KodrAus, I see. It seems like there is an issue with the CI then. |
|
Or an issue surfaced by CI I suppose. |
|
@dotnet-bot test this please |
|
@dotnet-bot test this please |
|
Turns out that ToSpan in the context of these tests is picking up an experimental version from S.B.Experimental that was implemented to be used with ReadOnlyBuffer. That version, used in this context, results in an infinite loop. Given what we are testing here, I don't really need to call ToSpan. |
|
@shiftylogic can we also fix that |
|
That ToSpan is working correctly for what it is supposed to be, which is specifically for ReadOnlyBuffer. I'm more interested in trying to figure out how any call to ReadableBuffer.ToSpan would ever resolve to using that extension method. There must be some implicit conversion happening. |
|
Oh, I didn't realize that ReadableBuffer actually implemented ISequence<ReadOnlyBuffer>. I'll take a look and see why the method ends up in an infinite loop. FYI, the correct solution might be to expose a ToSpan extension publicly that works specifically for ReadableBuffer, since there is a faster method than exists for the generic one. |
|
Created an issue for ToSpan (#1436). It doesn't block this merge since we aren't using it anymore. |
* Fixes to re-enable Pipeline tests that were skipped. * Simplify changes to pipeline tests. * PR Changes * Undoing previous change. * Changes from Pavel * ToSpan extension in this context is available. Picking up a bad one.
Fixes #1404