Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Fix ReadOnlySequence First#27691

Merged
ahsonkhan merged 2 commits into
dotnet:masterfrom
benaadams:ros-first
Mar 4, 2018
Merged

Fix ReadOnlySequence First#27691
ahsonkhan merged 2 commits into
dotnet:masterfrom
benaadams:ros-first

Conversation

@benaadams
Copy link
Copy Markdown
Member

@benaadams benaadams commented Mar 3, 2018

Seen in dotnet/corefxlab#2143

Don't have enough bandwidth on the train to download and install a fresh coreclr to get passed apicheck; but the test definitely fails before the change. Hopefully it passes after :)

/cc @ahsonkhan @davidfowl @pakrym

@benaadams
Copy link
Copy Markdown
Member Author

Centos.74.Amd64.Open:Release-x64
System.Net.Http.Functional.Tests 🔥

@dotnet-bot test Linux x64 Release Build

@davidfowl
Copy link
Copy Markdown
Member

LGTM

}

[Fact]
public void CanGetFirst()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We have other First tests right?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes; though not mismatched multi segment buffer sizes (which this is)

@benaadams
Copy link
Copy Markdown
Member Author

Don't have permission to rerun Tizen; and OSX is just generally broken

@ahsonkhan
Copy link
Copy Markdown

ahsonkhan commented Mar 3, 2018

This file isn't included in the test csproj (and it needs to be): https://github.com/dotnet/corefx/blob/master/src/System.Memory/tests/ReadOnlyBuffer/ReadOnlySequenceTests.TryGet.cs

And it needs to be updated to remove use of IMemoryList!

Can you try and get code coverage numbers too please?

var buffer = new ReadOnlySequence<byte>(bufferSegment1, 0, bufferSegment4, 200);

Assert.Equal(500, buffer.Length);
var length = 500;
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

nit: fix use of vars

Copy link
Copy Markdown

@ahsonkhan ahsonkhan left a comment

Choose a reason for hiding this comment

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

Otherwise, LGTM.

@ahsonkhan
Copy link
Copy Markdown

Don't have permission to rerun Tizen

What do you mean?

@dotnet-bot test Tizen armel Debug Build

@benaadams
Copy link
Copy Markdown
Member Author

dotnet bot ignores me in corefx for Tizen reruns (works in coreclr though). I think its a limited group because it gets so overloaded

Copy link
Copy Markdown
Member

@davidfowl davidfowl left a comment

Choose a reason for hiding this comment

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

LGTM

@benaadams
Copy link
Copy Markdown
Member Author

image

@ahsonkhan
Copy link
Copy Markdown

BuffersExtensions at 70% - we need to get that coverage up - I will add to https://github.com/dotnet/corefx/issues/26768

Anything from the ROS<T> coverage (~93%) that leaps out at you that is important to test?

@benaadams
Copy link
Copy Markdown
Member Author

BuffersExtensions.Write<T> is probably the main thing with 0% coverage; others don't look too exciting.

Some argument throws for .ctor validation (empty array). A couple exceptions I'm not sure can ever be hit, some non-segment paths (pure array/string/OwnedMemory cast paths)

@benaadams
Copy link
Copy Markdown
Member Author

added report to issue

@pakrym
Copy link
Copy Markdown

pakrym commented Mar 4, 2018

BuffersExtensions.Write<T> is covered by pipelines tests.

Assert.Equal(i, buffer.First.Length);
buffer = buffer.Slice(1);
length--;
Assert.Equal(length, buffer.Length);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I've would've also checked contents.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@benaadams can you add that?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Might not be soon. In airport, then in plane for 10 hrs 😀

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