Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.
/ corefx Public archive

Add tests for calling Span APIs via reflection to verify graceful failures. #28674

Merged
merged 8 commits into from
Mar 31, 2018

Conversation

ahsonkhan
Copy link

@ahsonkhan ahsonkhan self-assigned this Mar 31, 2018
@ahsonkhan ahsonkhan requested review from jkotas and a user March 31, 2018 01:36
Assert.Throws<TargetException>(() => ctor.Invoke(new object[] { new int[10], 1, 1 }));

ctor = type.GetConstructor(new Type[] { typeof(void*), typeof(int) });
Assert.Throws<TargetException>(() => ctor.Invoke(new object[] { null, 1 }));
Copy link
Member

Choose a reason for hiding this comment

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

Anything to validate about the TargetException's inner exception?

Copy link
Author

Choose a reason for hiding this comment

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

What should we validate about the inner exception? That it is null?

Copy link
Member

Choose a reason for hiding this comment

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

If the inner exception is null, then nevermind. I thought TargetException is often thrown from reflection when it's wrapping another exception, but maybe that's only true of TargetInvocationException?

{
Type type = typeof(MemoryExtensions);

MethodInfo method = type.GetMethod("AsSpan", new Type[] { typeof(string) });
Copy link
Member

Choose a reason for hiding this comment

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

Can we use nameof(MemoryExtensions.AsSpan) instead of manually typing the string? Same question for the rest of these.

Copy link
Author

Choose a reason for hiding this comment

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

What about methods like op_Equality?

Copy link
Member

Choose a reason for hiding this comment

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

Basically use nameof wherever you can. If you can't use it in some circumstances, then don't 😄

@jkotas
Copy link
Member

jkotas commented Mar 31, 2018

It is overkill to have the tests for all methods on Span. You should have just one method per interesting category:

  • constructor
  • instance method
  • static method
  • span argument
  • span return value

@ahsonkhan
Copy link
Author

ahsonkhan commented Mar 31, 2018

It is overkill to have the tests for all methods on Span.

I didn't add tests for all the methods. But I do see some duplication of the MemoryExtensions static methods. I can remove some of those.

  • constructor
  • instance method
  • static method
  • span argument
  • span return value

@jkotas
Copy link
Member

jkotas commented Mar 31, 2018

It would be useful to name the tests based on the interesting category that they are testing, not based on the method name that they happen to use to test it.

@jkotas
Copy link
Member

jkotas commented Mar 31, 2018

Thanks!

@jkotas
Copy link
Member

jkotas commented Mar 31, 2018

You will need to disable these tests on netfx.

@ahsonkhan ahsonkhan merged commit a7b59b0 into dotnet:master Mar 31, 2018
@ahsonkhan ahsonkhan deleted the AddReflectionTest branch March 31, 2018 04:25
@karelz karelz added this to the 2.1.0 milestone Apr 2, 2018
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
…lures. (dotnet/corefx#28674)

* Add tests for calling Span APIs via reflection to verify graceful failures.

* Add Span reflection tests to verify graceful failures

* Address PR feedback (test cleanup and rename)

* Fix up some test names

* Minor tweak: Reorder the tests

* Exclude running the reflection tests for portable span (netfx)

* Remove unused constant from csproj


Commit migrated from dotnet/corefx@a7b59b0
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.

[API Proposal] Add reflection support to byref-like types
4 participants