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

Conversation

@GrabYourPitchforks
Copy link
Member

Unit tests for dotnet/coreclr#22516.

These tests use private reflection to call into ASCIIUtility to test the method implementations. These methods serve as the workhorse routines for ASCIIEncoding. There are existing tests for ASCIIEncoding (see the project System.Text.Encoding.Tests), but for increased coverage and to exercise edge cases that ASCIIEncoding might be hiding I wanted to test the implementation directly. This should also serve as a future-proofing mechanism in case components other than ASCIIEncoding end up calling into it.

@GrabYourPitchforks
Copy link
Member Author

Note: The unit tests should pass right now because [slow] implementations of these methods already exist.

I ran these tests against the PR making its way through coreclr and confirmed that they succeed. Additionally, I ran separate tests against the following specific variants and confirmed correct functionality:

COMPLUS_ENABLEBMI2=0
COMPLUS_ENABLEBMI1=0
COMPLUS_ENABLEAVX=0
COMPLUS_ENABLESSE41=0
COMPLUS_ENABLESSE2=0

I also ifdefed out everything except the Vector<T>-only implementation (no hand-rolled hardware intrinsics) and confirmed correct functionality there as well.

/cc @tannergooding just in case he has any suggestions for additional scenarios I should be testing.

{
private const int SizeOfVector128 = 128 / 8;

// The delegate definitions and members below provide us access to CoreLib's internals.
Copy link
Member

Choose a reason for hiding this comment

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

We also have a project at https://github.com/dotnet/corefx/blob/master/src/Common/tests/Common.Tests.csproj that's used to test various common helpers. You could put these tests there instead of using reflection.

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting. Do you know how that test project works when faced with shared source files that contain #if BIT64 and similar?

Copy link
Member

Choose a reason for hiding this comment

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

It probably doesn't. :)

Copy link
Member Author

Choose a reason for hiding this comment

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

The curse of nuint strikes again. 😈

@GrabYourPitchforks
Copy link
Member Author

I'll check this in for now since it's just unit tests (no runtime changes) and we could benefit from the increased coverage. Will leave it open for further reviews and so that we can also figure out how to make the Common project support multiple archs properly.

@GrabYourPitchforks GrabYourPitchforks merged commit c50a04a into dotnet:master Mar 23, 2019
@GrabYourPitchforks GrabYourPitchforks deleted the ascii_20 branch March 23, 2019 23:00
@karelz karelz added this to the 3.0 milestone Apr 1, 2019
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants