Some assertions are not including variable names in assertion message #1435

Serg046 · 2020-11-21T15:52:33Z

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.

dennisdoomen

That's quite an impressive PR. To be fair, I would not have invested the time to support all the misuse of the language in your examples. But that's the awesomeness of the community ❤

dennisdoomen · 2020-11-23T06:31:33Z

Tests/FluentAssertions.Specs/Execution/CallerIdentifierSpecs.cs

+        }
+
+        [Fact]
+        public void When_variable_is_not_captured_it_should_use_the_variable_name()


🤔 I don't get this scenario.

It was added when I started with Mono.Cecil. That time such test was important, now it is not. I will just remove this test and maybe similar ones

I've removed this one - but I'm not sure what the similar ones that can be removed are

dennisdoomen · 2020-11-23T06:34:38Z

Tests/FluentAssertions.Specs/Execution/CallerIdentifierSpecs.cs

+        }
+
+        [Fact]
+        public void When_field_is_the_caller_it_should_use_the_field_name()


🙃 I think it's better to change the term caller into subject or something like that.

dennisdoomen · 2020-11-23T06:36:09Z

Tests/FluentAssertions.Specs/Execution/CallerIdentifierSpecs.cs

+        }
+
+        [Fact]
+        public void When_method_name_contains_get_it_should_not_remove_the_prefix()


🤔 Why is this an expected scenario?
🙃 The name is rather difficult to read since you refer to some literal term. I would rephrase contains_get to contains_getter_prefix to keep it readable.

The whole test could be removed because the current code base just rely on symbols, no need to distinguish properties and methods

dennisdoomen · 2020-11-23T06:41:04Z

Tests/FluentAssertions.Specs/Execution/CallerIdentifierSpecs.cs

+
+            // Assert
+            act.Should().Throw<XunitException>()
+                .WithMessage("*Expected foo.GetFoo(test1).GetFooStatic(\"test\"+2).GetFoo(foo.Field) to be <null>*");


🤔 I think it would be better to only capture the GetFoo(fool.Field), but I guess it's going to be too hard to figure that out.

I think it is not that hard. We could look for the last dot which is not part of arguments list. The reason why I didn't do it like you suggest is that I saw the current behaviour better. Just let me know if you think it is worth doing. I can do it right here (guess it is better) or implement it as a separate PR.

@dennisdoomen I would be OK with leaving this as is as long as you are - I kind of like the extra context (although if it were too long via chained method calls, it could we unwieldy - then again with EF this could end up showing .ToList() which wouldn't be helpful).

dennisdoomen · 2020-11-23T06:42:03Z

Tests/FluentAssertions.Specs/Execution/CallerIdentifierSpecs.cs

+        }
+
+        [Fact]
+        public void When_method_contains_parameters_it_should_add_them_to_caller()


🔧 The proper term is arguments, not parameters.

dennisdoomen · 2020-11-23T06:56:41Z

Src/FluentAssertions/CallerStatementBuilder.cs

+            Result Handle(char symbol);
+        }
+
+        private class QuotesHandler : IHandler


🔧 I think it might be a good idea to move the CallerIdentifier and all this new code into a folder CallerIdentification. Especially because we also prefer to have separate files per class over private classes.

dennisdoomen · 2020-11-23T06:58:18Z

Src/FluentAssertions/CallerIdentifier.cs

+            }
+
+            var sb = new CallerStatementBuilder();
+            CallerStatementBuilder.Result state;


🔧 If your enum needs to be prefixed by another class, treating it as a nested type is not a good idea.

dennisdoomen · 2020-11-23T06:59:16Z

Src/FluentAssertions/CallerStatementBuilder.cs

+
+        public override string ToString() => statement.ToString();
+
+        public enum Result


🤔 I don't understand the different values. E.g. what's the difference between Done and Handled? Maybe adding some XML comments will help.

Yes, I remember that I was not happy by this thing. The answer is in this method:

public Result Append(string symbols) { Result result = Result.InProgress; using var symbolEnumerator = symbols.GetEnumerator(); while (symbolEnumerator.MoveNext() && result != Result.Done) { result = Result.InProgress; using var handlerEnumerator = handlers.GetEnumerator(); while (handlerEnumerator.MoveNext() && result == Result.InProgress) { result = handlerEnumerator.Current.Handle(symbolEnumerator.Current); } } return result; }

I have 2 cycles and 2 flags for exit. I will try to refactor it somehow, anyway I need to move some things as suggested above.

@Serg046 Did you have any thoughts/suggestions you could share on this? I don't mind making the changes, there's just a lot going on 😛

@mdentremont, the only suggestion is to refactor it somehow. I just remember that processing was line by line where Handled means that the line is processed and we need to switch next.

Ah no worries. I don't think it's all that bad as it stands - it takes a second to wrap your head around, but then it's decently straightforward.

If you have any refactoring ideas you'd like to apply, maybe just make them in a future PR 👍

dennisdoomen · 2020-11-23T07:03:32Z

Src/FluentAssertions/CallerStatementBuilder.cs

+                return isQuoteContext ? Result.Handled : Result.InProgress;
+            }
+
+            private bool IsAtEscaped()


🔧 As mentioned before, I would rename this to IsVerbatim

dennisdoomen · 2020-11-23T07:04:12Z

Src/FluentAssertions/CallerStatementBuilder.cs

+                if (statement.Length > 1)
+                {
+                    var idx = statement.Length - 1;
+                    return statement[idx--] == '$'


❌ Please don't mix increments like these. People always forget in what order things happen.

dennisdoomen · 2020-12-23T06:20:42Z

@Serg046 did you give up on this?

Serg046 · 2020-12-23T09:34:17Z

I am just waiting for the holidays. If you still think we could merge something like this, I will complete it soon.

dennisdoomen · 2020-12-23T09:38:31Z

@jnyrup what do you think?

jnyrup · 2020-12-23T14:56:07Z

I think it looks quite interesting. I just haven't taken the time to digest this fully.

I was mostly concerned about other PRs keeping caller identification lazy.

rygwdn · 2021-05-13T13:16:43Z

Is there still interest in this PR? I would love to see #1353 fixed!

dennisdoomen · 2021-05-13T13:28:06Z

Is there still interest in this PR? I would love to see #1353 fixed!

If somebody is willing to finalize the PR, then yes.

- Remove unneeded test - Fix wording & variable names

Merge develop & address most PR feedback for issue fluentassertions#1353

dennisdoomen

👍🏻 Impressive piece of work
❌ Please make everything related to caller identification internal.

Src/FluentAssertions/CallerStatementBuilder.cs

Src/FluentAssertions/CallerIdentification/IHandler.cs

Src/FluentAssertions/CallerIdentification/HandlerResult.cs

Src/FluentAssertions/CallerIdentification/QuotesHandler.cs

Src/FluentAssertions/CallerIdentification/ShouldCallHandler.cs

jnyrup · 2021-06-07T17:18:58Z

Src/FluentAssertions/CallerIdentification/QuotesHandler.cs

+            return isQuoteContext ? HandlerResult.Handled : HandlerResult.InProgress;
+        }
+
+        private bool IsVerbatim()


@$"" is also a valid interpolated verbatim string.

I think that's what it is doing?

Ahh sorry, I meant that $@"" is also a valid interpolated verbatim string.

ahh OK - I'll try to get this resolved today hopefully.

jnyrup · 2021-06-07T17:35:18Z

docs/_pages/releases.md

 * Better parameter checking of `MethodInfoSelectorAssertions` - [#1569](https://github.com/fluentassertions/fluentassertions/pull/1569)
 * Better parameter checking of `XDocumentAssertions`, `XElementAssertions` and `XAttributeAssertions` - [#1564](https://github.com/fluentassertions/fluentassertions/pull/1564)
 * In a chained assertion API call, a second call to `ForCondition` should not even evaluate its lambda when the previous assertion failed - [#1587](https://github.com/fluentassertions/fluentassertions/pull/1587)
+* Improved caller name determination by supporting multiple lines, comments and semicolons - [#1435](https://github.com/fluentassertions/fluentassertions/pull/1435).


Beta 1 has shipped, this will be part of 6.0.0 xxx

mdentremont · 2021-06-17T23:08:42Z

👍🏻 Impressive piece of work
❌ Please make everything related to caller identification internal.

Done

…older

- Add docs - IHandler => IParsingStrategy - Pass StringBuilder/statement in Parse method rather than handing it to strategy constructors (easier to follow) - Handled => GoToNextSymbol - strategies => priorityOrderedParsingStrategies to make it more clear that the order is important

- Having "WithMessage("*..*")" was causing failing tests to pass (this seems odd, maybe WithMessage isn't working correctly?)

- note: a trailing whitespace character was auto-trimmed by my IDE, I left the change because it felt dirty :P

mdentremont · 2021-06-18T02:16:17Z

@dennisdoomen @jnyrup Thanks so much for the feedback you two!

While addressing the feedback, I noticed that the tests that did WithMessage("*...*") were being passed even though the parts inside the wildcards were not in the message (this sounds like a possible WithMessage bug?).

I also noticed some edge cases while reading through the code, so I added new tests to confirm my suspicions (the tests failed).

I've made a decent amount of refactoring, and have also fixed all of the broken tests (and of course addressed as much of the review feedback as I could, and replied where I couldn't).

Cheers!

- This is why we added NotifyEndOfLineReached()

- Use guard clauses & extract naming variables

Address more review feedback

dennisdoomen

Some unit tests are failing...

- I'm somewhat grasping for straws on this one, my IDE wants to suppress using ReSharper commments, and won't offer to suppress in this manner

Issue 1353 (hopefully the last set of changes)

jnyrup · 2021-07-14T18:19:00Z

Src/FluentAssertions/CallerIdentification/ShouldCallParsingStrategy.cs

    internal class ShouldCallParsingStrategy : IParsingStrategy
    {
-        private const string ShouldCall = ".Should()";
+        private const string ShouldCall = ".Should(";


Is there any disadvantage to using .Should( over .Should()?
I imagine Should is only called with an argument in our tests when we fake timing.

It think it seems reasonable. This way it also still works if somebody were to overload .Should via an extension method (not sure why they would though).

Serg046 added 5 commits November 10, 2020 00:40

Support callers on multiple lines

3633c6c

Add handlers

9f38bb8

Support comments

4cb1632

Merge branch 'develop' into issue-1353

b80cb8d

Add a release note

5630f43

Serg046 changed the title ~~Issue 1353~~ Some assertions are not including variable names in assertion message Nov 21, 2020

dennisdoomen requested changes Nov 23, 2020

View reviewed changes

Matt d'Entremont added 8 commits June 3, 2021 10:44

Merge branch 'develop' into issue-1353

889a591

Split private classes into separate files

7487fd4

Rename as suggested in PR

bcbab77

Do not mix increment/decrement styles

c879448

Fix "if statement can be simplified" warnings

40fef54

Test cleanup

21fd228

- Remove unneeded test - Fix wording & variable names

Remove redundant

36231d2

Do expose HandlerResult to CallerIdentifier

59a4d1b

mdentremont mentioned this pull request Jun 3, 2021

Merge develop & address most PR feedback for issue #1353 Serg046/fluentassertions#1

Merged

Merge pull request #1 from mdentremont/issue-1353

3b7f154

Merge develop & address most PR feedback for issue fluentassertions#1353

dennisdoomen requested changes Jun 6, 2021

View reviewed changes

jnyrup reviewed Jun 7, 2021

View reviewed changes

Use internal for the caller identification stuff

293f74e

Matt d'Entremont added 3 commits June 17, 2021 20:10

Move caller identification-related code to the CallerIdentification f…

ef93c89

…older

Fix broken WithMessage calls

66d6fad

- Having "WithMessage("*..*")" was causing failing tests to pass (this seems odd, maybe WithMessage isn't working correctly?)

Matt d'Entremont added 4 commits June 17, 2021 23:02

Add a couple new tests (all fail without upcoming commits)

53c8172

Update parsing strategies to fix broken tests

ae1f8b3

Rename variables

5e0c023

Updated releases as instructed

9689fe6

- note: a trailing whitespace character was auto-trimmed by my IDE, I left the change because it felt dirty :P

dennisdoomen mentioned this pull request Jun 18, 2021

Address more review feedback Serg046/fluentassertions#2

Merged

Matt d'Entremont and others added 5 commits June 18, 2021 08:46

Add more edge case tests just in case

4110cc1

Apply a couple Resharper warning fixes/suggestions

ec60351

SingleLineCommentParsingStrategy will never encounter \n

56e3c40

- This is why we added NotifyEndOfLineReached()

Tidy-ups for SingleLineCommentParsingStrategy

a74b648

- Use guard clauses & extract naming variables

Merge pull request #2 from mdentremont/issue-1353

55781e1

Address more review feedback

dennisdoomen approved these changes Jun 29, 2021

View reviewed changes

dennisdoomen requested a review from jnyrup June 29, 2021 06:48

Matt d'Entremont and others added 4 commits June 29, 2021 10:50

Handle more types of verbatim strings

08bb4b0

Fix edge case where argument is passed to .Should() by being less strict

e42534a

Suppress warnings

5918414

- I'm somewhat grasping for straws on this one, my IDE wants to suppress using ReSharper commments, and won't offer to suppress in this manner

Merge pull request #3 from mdentremont/issue-1353

2e9e5bb

Issue 1353 (hopefully the last set of changes)

jnyrup reviewed Jul 14, 2021

View reviewed changes

jnyrup approved these changes Jul 14, 2021

View reviewed changes

jnyrup merged commit 8d88ec7 into fluentassertions:develop Jul 19, 2021

dennisdoomen mentioned this pull request Aug 10, 2021

Merge missed improvements from develop #1639

Merged


		public override string ToString() => statement.ToString();

		public enum Result

Some assertions are not including variable names in assertion message #1435

Some assertions are not including variable names in assertion message #1435

Uh oh!

Conversation

Serg046 commented Nov 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

IMPORTANT

Uh oh!

dennisdoomen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dennisdoomen commented Dec 23, 2020

Uh oh!

Serg046 commented Dec 23, 2020

Uh oh!

dennisdoomen commented Dec 23, 2020

Uh oh!

jnyrup commented Dec 23, 2020

Uh oh!

rygwdn commented May 13, 2021

Uh oh!

dennisdoomen commented May 13, 2021

Uh oh!

dennisdoomen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Serg046 commented Nov 21, 2020 •

edited

Loading

mdentremont Jul 14, 2021 •

edited

Loading