Make type assertions more robust #1550

eNeRGy164 · 2021-05-09T11:20:52Z

Inspired by #1039

Check for Subject to be null
Check parameters types for null (or empty)
Check enum parameters types for out-of-range values
Add covering unit tests
Add possible exceptions to XmlDoc
Try to get more consistent function/test layout
Try to get more consistent documentation
~~replace "to not exist" with "not to exist", both are valid but this feels more fluent to me, but maybe it is just taste.~~
Condense tests

Status:

IMPORTANT

The code complies with the Coding Guidelines for C#.
The changes are covered by a new or existing set of unit tests which follow the Arrange-Act-Assert syntax such as is used in this example.
If the contribution adds a feature or fixes a bug, please update the release notes, which are published on the website.
If the contribution changes the public API the changes needs to be included by running AcceptApiChanges.ps1/AcceptApiChanges.sh.
If the contribution affects the documentation, please include your changes in this pull request so the documentation will appear on the website.

eNeRGy164 · 2021-05-09T22:13:21Z

Questions:

Should this be split up in multiple commits? If so, any preferences?
The build fails on a changed API, but there is no change in the files if I run it? Only Guard methods have been added.

dennisdoomen · 2021-05-10T08:05:55Z

Should this be split up in multiple commits? If so, any preferences?

I love a clean history and encourage people to provide clean PRs (see also my blog post on this).

The build fails on a changed API, but there is no change in the files if I run it? Only Guard methods have been added.

I think I have causes that. Saw the same change in a PR from @jnyrup. I'll take a look.

jnyrup · 2021-05-10T15:41:05Z

I included a fix for the failing approval tests in #1552.

eNeRGy164 · 2021-05-11T08:13:50Z

I have a couple of points left:

Should a couple of these commits be split to different PR's? I can imagine like 1. Robustness, 2. Code/doc structure lay-out
I discovered that explicit method assertions do not have the parameters listed in the message, can I add these here, or should this be a separate PR?
I mentioned the "expected x to not exist", "expected x to not be derived from" vs "expected x not to exist", "expected x not to be derived from". It's currently out due to the splitting, any opinions on this?
What should be added to the release notes?
I still feel the urge to standardize the layout of the Arrange/Act/Assert and .Should() layout (newlines, alignment) through the whole file, any thoughts?

dennisdoomen · 2021-05-11T09:02:32Z

Should a couple of these commits be split to different PR's? I can imagine like 1. Robustness, 2. Code/doc structure lay-out

Nah. Separate commits is clear enough. Will merge them as is.

I discovered that explicit method assertions do not have the parameters listed in the message, can I add these here, or should this be a separate PR?

What do you mean?

I mentioned the "expected x to not exist", "expected x to not be derived from" vs "expected x not to exist", "expected x not to be derived from". It's currently out due to the splitting, any opinions on this?

You mean to improve the failure messages? If it's going to affect a lot of tests (and thus files), I would keep that in a separate PR?

What should be added to the release notes?

You could mention something like "Improved the failure messages if the library is not used correctly"?

I still feel the urge to standardize the layout of the Arrange/Act/Assert and .Should() layout (newlines, alignment) through the whole file, any thoughts?

If it involves a lot of changes, I recommend doing it in a separate commit or separate PR

eNeRGy164 · 2021-05-11T09:37:12Z

I discovered that explicit method assertions do not have the parameters listed in the message, can I add these here, or should this be a separate PR?

What do you mean?

With

type.Should().HaveMethod("NonExistentMethod", new[] { typeof(int), typeof(Type) })

you get

Expected method *.ClassWithNoMembers.NonExistentMethod(System.Int32, System.Type) to exist

With

type.Should().HaveExplicitMethod(interfaceType, "ImplicitMethod", new[] { typeof(int), typeof(Type) });

you get

Expected {interface} to explicitly implement *.IExplicitInterface.ImplicitMethod, ...

instead of

Expected {interface} to explicitly implement *.IExplicitInterface.ImplicitMethod(System.Int32, System.Type), ...

This is the only method related assertion that does not show the whole signature.

I mentioned the "expected x to not exist", "expected x to not be derived from" vs "expected x not to exist", "expected x not to be derived from". It's currently out due to the splitting, any opinions on this?

You mean to improve the failure messages? If it's going to affect a lot of tests (and thus files), I would keep that in a separate PR?

Currently only 2 files, TypeAssertions + Specs :)

But it is also import to know if you think it is an improvement to change the order of the words.

eNeRGy164 · 2021-05-11T13:14:39Z

I think I'm done for know, other then await you review comments of course.

For style/layout issues I started a discussion: #1555

For fixing argument issues in other types I will create separate PR's in the future (maybe a little smaller 😛)

dennisdoomen · 2021-05-11T16:12:20Z

This is the only method related assertion that does not show the whole signature.

Knock yourself out ;-)

But it is also import to know if you think it is an improvement to change the order of the wo

Same here ;-)

eNeRGy164 · 2021-05-11T17:31:29Z

Now with a lot of line breaks :)

eNeRGy164 · 2021-05-12T11:07:40Z

This one should be ready for review, @dennisdoomen @jnyrup.
Won't take much time, small change only 🤣

Src/FluentAssertions/Common/Guard.cs

Src/FluentAssertions/Types/TypeAssertions.cs

Tests/FluentAssertions.Specs/Types/TypeAssertionSpecs.cs

docs/_pages/releases.md

dennisdoomen · 2021-05-12T13:09:59Z

Tests/FluentAssertions.Specs/Types/TypeAssertionSpecs.cs


        [Fact]
-        public void When_an_unrelated_type_it_fails_with_a_useful_message()
+        public void When_an_unrelated_type_it_fails()


🤔 This doesn't read very natural. Same for the other names.

Although I agree, I only removed the with_a_useful_message part on existing tests.

I won't mind looking at better language in both test names and assertion messages of the existing 281 unit tests and 57 assertion methods, but I don't think we should add more to this PR 🏳️

Tests/FluentAssertions.Specs/Types/TypeAssertionSpecs.cs

dennisdoomen · 2021-05-12T13:26:18Z

Won't take much time, small change only

I can't promise I kept as thorough as I was in the beginning. But next time, splitting this in separate PRs will make it more bearable.

eNeRGy164 · 2021-05-12T14:42:38Z

Won't take much time, small change only

I can't promise I kept as thorough as I was in the beginning. But next time, splitting this in separate PRs will make it more bearable.

Yeah, I will do better next time :)

jnyrup · 2021-05-12T18:13:10Z

Done reading, I have nothing to add.
I stopped reviewing tests, when the GH couldn't handle what seems to be reordering of tests.

Feel free to delay the work about making assertions robust against being wrapped in an AssertionScope to another PR.

This remove some duplicate tests and shortens a lot of test names

* Standardize on because(args) and validation * No interpolation * Replace namespaces with wildcard * Reduce usage of wildcards * Less concatenation

eNeRGy164 marked this pull request as ready for review May 9, 2021 22:12

eNeRGy164 mentioned this pull request May 11, 2021

Missing null checks #1039

Open

dennisdoomen requested changes May 12, 2021

View reviewed changes

eNeRGy164 added 10 commits May 13, 2021 00:20

Add guard methods for Enum Argument OutOfRange check

5285c80

Make TypeAssertions more robust

44b8868

Organize structure of conditions

b5b8759

Make XmlDoc more consistent and improve some text

34404bb

Always test for because(args)

af15e2e

This remove some duplicate tests and shortens a lot of test names

Simplify Type enumeration

16e509e

Cleanup assertion messages

d0085c9

* Standardize on because(args) and validation * No interpolation * Replace namespaces with wildcard * Reduce usage of wildcards * Less concatenation

Add parameters to explicit method signature in messages

cff7906

Apply 130 char break on TypeAssertion(Spec)s

98f9efa

Make assertions work correctly when wrapped in an AssertionScope

51487ec

dennisdoomen approved these changes May 14, 2021

View reviewed changes

dennisdoomen requested a review from jnyrup May 14, 2021 05:29

jnyrup approved these changes May 14, 2021

View reviewed changes

dennisdoomen merged commit 75e7396 into fluentassertions:develop May 14, 2021

eNeRGy164 deleted the robust-type-assertions branch May 22, 2021 13:14

Make type assertions more robust #1550

Make type assertions more robust #1550

Uh oh!

Conversation

eNeRGy164 commented May 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

IMPORTANT

Uh oh!

eNeRGy164 commented May 9, 2021

Uh oh!

dennisdoomen commented May 10, 2021

Uh oh!

jnyrup commented May 10, 2021

Uh oh!

eNeRGy164 commented May 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dennisdoomen commented May 11, 2021

Uh oh!

eNeRGy164 commented May 11, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eNeRGy164 commented May 11, 2021

Uh oh!

dennisdoomen commented May 11, 2021

Uh oh!

eNeRGy164 commented May 11, 2021

Uh oh!

eNeRGy164 commented May 12, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dennisdoomen May 12, 2021

Choose a reason for hiding this comment

Uh oh!

eNeRGy164 May 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

dennisdoomen commented May 12, 2021

Uh oh!

eNeRGy164 commented May 12, 2021

Uh oh!

jnyrup commented May 12, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

eNeRGy164 commented May 9, 2021 •

edited

Loading

eNeRGy164 commented May 11, 2021 •

edited

Loading

eNeRGy164 commented May 11, 2021 •

edited

Loading

eNeRGy164 May 13, 2021 •

edited

Loading