Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consider types assignable to open generics (#954) #955

Merged
merged 7 commits into from Nov 8, 2018

Conversation

mdonoughe
Copy link
Contributor

@mdonoughe mdonoughe commented Oct 31, 2018

I think I did this right, at least for BeAssignableTo. Base types and interfaces are searched for matching generic types.

Should BeDerivedFrom do the same thing but disallowing interfaces?

#954

IMPORTANT

Copy link
Member

@dennisdoomen dennisdoomen left a comment

Only minor comments and suggestions. Feel free to ignore them or apply then. I use these emojis.

Src/FluentAssertions/Types/TypeAssertions.cs Outdated Show resolved Hide resolved
if (subjectInfo.IsInterface && subjectInfo.IsGenericType &&
subjectInfo.GetGenericTypeDefinition().IsSameOrEqualTo(definition))
{
return true;
Copy link
Member

@dennisdoomen dennisdoomen Oct 31, 2018

Choose a reason for hiding this comment

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

🙃 I personally find all these intermediate return statements a more difficult to read.

Copy link
Contributor Author

@mdonoughe mdonoughe Oct 31, 2018

Choose a reason for hiding this comment

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

Is this better now?

Src/FluentAssertions/Types/TypeAssertions.cs Outdated Show resolved Hide resolved
@@ -484,6 +484,65 @@ public void When_an_unrelated_type_instance_it_fails_with_a_useful_message()
.WithMessage($"*{typeof(DummyImplementingClass)} to be assignable to {typeof(DateTime)}*failure message*");
}

[Fact]
public void When_constructed_of_open_generic_it_succeeds()
Copy link
Member

@dennisdoomen dennisdoomen Oct 31, 2018

Choose a reason for hiding this comment

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

🙃 We normally use the When_...._it_should_.... format for all our tests.
🤔 I don't get the when constructed. We're not constructing anything here, right?

Copy link
Contributor Author

@mdonoughe mdonoughe Oct 31, 2018

Choose a reason for hiding this comment

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

Microsoft uses the term "constructed" when referring to a generic type that has its arguments filled in: https://docs.microsoft.com/en-us/dotnet/api/System.Type.IsConstructedGenericType?view=netframework-4.7.2 . I hadn't seen it before either.

Copy link
Member

@dennisdoomen dennisdoomen Nov 1, 2018

Choose a reason for hiding this comment

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

I guess we're missing some kind of context for a group of specs so that we don't have to repeat the cause and effect.

}

[Fact]
public void When_implementation_of_open_generic_interface_it_succeeds()
Copy link
Member

@dennisdoomen dennisdoomen Oct 31, 2018

Choose a reason for hiding this comment

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

🙃 It feels like the name is missing a verb or something.

Copy link
Contributor Author

@mdonoughe mdonoughe Oct 31, 2018

Choose a reason for hiding this comment

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

I was trying to emulate the "When_its_own_type_instance_it_succeeds" pattern the other BeAssignableTo methods were following without making the names super long.

@mdonoughe
Copy link
Contributor Author

@mdonoughe mdonoughe commented Oct 31, 2018

I've added in support for BeDerivedFrom too.

jnyrup
jnyrup approved these changes Nov 1, 2018
Copy link
Member

@jnyrup jnyrup left a comment

Thanks for taking up this issue 👍
If you have appetite for more, we need to get the following assertions consistent with respect to open generics.
Such that e.g. obj.Should().BeAssignableTo(typeof(IInterface<>)) and obj.GetType().Should().BeAssignableTo(typeof(IInterface<>)) have the same behavior.

TypeAssertions

  • [Not]Implement
  • [Not]BeDerivedFrom
  • [Not]BeAssignableTo

ReferenceTypeAssertions

  • [Not]BeOfType
  • [Not]BeAssignableTo

Src/FluentAssertions/Types/TypeAssertions.cs Outdated Show resolved Hide resolved
Src/FluentAssertions/Types/TypeAssertions.cs Outdated Show resolved Hide resolved
Src/FluentAssertions/Types/TypeAssertions.cs Outdated Show resolved Hide resolved
@mdonoughe
Copy link
Contributor Author

@mdonoughe mdonoughe commented Nov 2, 2018

BeOfType already had open generics.

Is there a convention for both BeAssignableTos to use the same logic?

@jnyrup
Copy link
Member

@jnyrup jnyrup commented Nov 2, 2018

Yes, but NotBeOfType does not.

I'm not sure I understand your question.
If you're asking where to put shared logic, I would put it in TypeExtensions.cs.

@jnyrup
Copy link
Member

@jnyrup jnyrup commented Nov 5, 2018

Don't know if this is WIP, but BeAssignableTo<T> should probably call BeAssignableTo(Type) instead?

@mdonoughe
Copy link
Contributor Author

@mdonoughe mdonoughe commented Nov 6, 2018

It probably should, but it doesn't seem to make any difference since you can't pass open generics to methods like that. I assumed it did already.

@mdonoughe
Copy link
Contributor Author

@mdonoughe mdonoughe commented Nov 6, 2018

Actually should it? BeAssignableTo<T> casts the result to T, so for cases where it wouldn't match BeAssignableTo (shouldn't be any?) it would fail to cast even if it relied on BeAssignableTo to do the assertion.

@dennisdoomen
Copy link
Member

@dennisdoomen dennisdoomen commented Nov 7, 2018

@jnyrup can this be merged?

@jnyrup
Copy link
Member

@jnyrup jnyrup commented Nov 7, 2018

It looks very good now and feature wise could be merge.
But I would like to have both a positive and a negative test for BeAssignableTo, NotBeAssignableTo and NotBeOfType from ReferenceTypeAssertions when the expectation is an open generic.

@dennisdoomen
Copy link
Member

@dennisdoomen dennisdoomen commented Nov 7, 2018

Maybe it's better to request changes then. It makes the state of the PR more visual.

Copy link
Member

@jnyrup jnyrup left a comment

I would like to have both a positive and a negative test for BeAssignableTo, NotBeAssignableTo and NotBeOfType from ReferenceTypeAssertions when the expectation is an open generic.

jnyrup
jnyrup approved these changes Nov 8, 2018
Copy link
Member

@jnyrup jnyrup left a comment

@mdonoughe You can ignore the single failing test.
Testing timing related test is sometimes unstable, and all except one target framework that test passed.
I'm very satisfied now, so if you're good to go, I'll hit the merge button

@dennisdoomen dennisdoomen merged commit a9abfa8 into fluentassertions:master Nov 8, 2018
1 check passed
@mdonoughe mdonoughe deleted the open-generics branch Nov 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants