Redesign AssertionScope and how it travels over chained calls #2539

dennisdoomen · 2024-01-06T09:56:35Z

Attempts to fix #2253

AssertionScope is only used by consumers as they do now and by certain APIs such as BeEquivalentTo
Every Should method will create a new object called Assertion (but I'm open to a better name) that contains the context of the assertion and a link to the scope. When no ambient AssertionScope is available, it will be a new one that is created at that point. This is different from v6 where we create a new scope per call to Execute.Assertion.
This Assertion instance will be explicitly passed to all the assertion classes (like StringAssertions).
No public APIs will depend on AssertionScope.Current anymore
Most of the APIs that are now part of AssertionScope will need to move to Assertion.
A method like GenericCollectionAssertions.ContainSingle returns a new version of AndWhich that takes a postfix such as [0]. Only when the Which property of that intermediate class is used, the current Assertion is updated with that postfix and registered on the current thread (using AsyncLocal). This ensures that the Should call chained on Which will pick-up that Assertion instance and continue the assertion with the right caller identifier. See for example Chaining_something_should_do_something
This should also allow us to drop ClearExpectations
Also provides a nice split between the responsibility of AssertionScope as a way to group related assertions and its responsibilities as the internal assertion API

IMPORTANT

If the PR touches the public API, the changes have been approved in a separate issue with the "api-approved" label.
The code complies with the Coding Guidelines for C#.
The changes are covered by unit tests which follow the Arrange-Act-Assert syntax and the naming conventions such as is used in these tests.
If the PR adds a feature or fixes a bug, please update the release notes with a functional description that explains what the change means to consumers of this library, which are published on the website.
If the PR changes the public API the changes needs to be included by running AcceptApiChanges.ps1 or AcceptApiChanges.sh.
If the PR affects the documentation, please include your changes in this pull request so the documentation will appear on the website.
- Please also run ./build.sh --target spellcheck or .\build.ps1 --target spellcheck before pushing and check the good outcome

github-actions · 2024-01-06T10:01:12Z

Qodana for .NET

6 new problems were found

Inspection name	Severity	Problems
`Parameter hides member`	🔶 Warning	2
`Get-only auto-property is never assigned`	🔶 Warning	2
`Redundant using directive`	🔶 Warning	1
`Add/remove 'this.' qualifier`	◽️ Notice	1

💡 Qodana analysis was run in the pull request mode: only the changed files were checked
☁️ View the detailed Qodana report

Contact Qodana team

Contact us at qodana-support@jetbrains.com

Or via our issue tracker: https://jb.gg/qodana-issue
Or share your feedback: https://jb.gg/qodana-discussions

jnyrup · 2024-01-08T17:54:45Z

Had a rather quick look at this and it looks very promising 🤩
Nothing caught my attention as being bad, regressing the current behavior or having pitfalls.

dennisdoomen · 2024-01-08T18:09:55Z

Had a rather quick look at this and it looks very promising 🤩
Nothing caught my attention as being bad, regressing the current behavior or having pitfalls.

Shall I continue with the real implementation? Or do you want to take a deeper look first?

dennisdoomen · 2024-02-25T08:08:16Z

I'm wondering whether the new Assertion class in this redesign is a good name, given that we also have all the classes postfixed with Assertions. ChatGPT and Gemini suggested FluentAssertion, FluentAssertionBuilder, AssertChain, Verifier and FluentVerifier. Any thoughts? For reference, it is used in

Assertion.GetOrCreate()
  .BecauseOf(because, becauseArgs)
  .WithExpectation("Expected {context:collection} to be empty{reason}, ")
  .Given(() => singleItemArray)
  .ForCondition(subject => subject is not null)
  .FailWith("but found <null>.")
  .Then
  .ForCondition(subject => subject.Length == 0)
  .FailWith("but found at least one item {0}.", singleItemArray);

I do like FluentAssertion myself. I also like FluentAssertionBuilder, but it's not like we're really building something. The conditions are evaluated right away. AssertionChain is a better fit from a functional perspective, but sounds very much like AssertionScope, which is something completely different.

IT-VBFK · 2024-02-25T10:03:41Z

FluentAssertionProvider 🤔

jnyrup · 2024-02-25T10:46:15Z

AssertionEvaluator?
In any case I would avoid the Fluent prefix as that is about how it does something compared to what it does.

codymullins · 2024-02-25T14:07:12Z

Assertion makes sense to me. AssertionBuilder does seem to map more closely to what it’s doing, though yes it isn’t strictly speaking a builder.

How would this be used by external consumers, if at all? I have only really used Fluent Assertions via .Should(…)

edit - reading the PR description more clearly, FluentAssertion is probably just fine since this seems to be a mostly internal API…

obiwanjacobi · 2024-02-25T16:53:41Z

If the conditions are evaluated right away I would think Assert and then maybe rename the GetOrCreate() function(s) ...?
(I have no knowledge of the internals and the constructs used - so maybe this is bogus)

jscarle · 2024-02-25T18:57:01Z

AssertionContext?

dennisdoomen · 2024-02-27T13:00:07Z

How would this be used by external consumers, if at all? I have only really used Fluent Assertions via .Should(…)

It's used both as the internal engine as well as by anybody that builds extensions. See also https://fluentassertions.com/extensibility/#building-your-own-extensions

If the conditions are evaluated right away I would think Assert and then maybe rename the GetOrCreate() function(s) ...?
(I have no knowledge of the internals and the constructs used - so maybe this is bogus

Assert is a verb and not a noun, so that wouldn't really work

AssertionEvaluator?

It doesn't evaluate an assertion. You're building and executing a, potentially, chained assertion.

AssertionContext?

It is also not the context of an assertion. It's doing the assertion work.

PS. For reference, I've updated the original question with a more elaborate example.

jscarle · 2024-02-27T14:22:19Z

Considering you already have Verifier and FluentVerifier, wouldn't a natural fit be Asserter and FluentAsserter?

ITaluone · 2024-02-27T15:39:03Z

It is also not the context of an assertion. It's doing the assertion work.

AssertionWorker 🤔

vbreuss · 2024-02-27T15:54:32Z

What about AssertThat?

This way, your chain would read almost like an English sentence

assertThat
.BecauseOf(because, becauseArgs)
.WithExpectation("Expected {context:collection} to be empty{reason}, ")
.Given(() => singleItemArray)
.ForCondition(subject => subject is not null)
.FailWith("but found <null>.")
.Then
.ForCondition(subject => subject.Length == 0)
.FailWith("but found at least one item {0}.", singleItemArray);

victorvogelpoel · 2024-03-01T16:45:34Z

Something with 'Pipeline' or 'Predicate'?

dennisdoomen · 2024-03-02T16:28:00Z

Hmm, I kind of starting to like AssertionChain. The Assertion.GetOrCreate would become AssertionChain.ContinueOrStart

github-actions · 2024-03-31T09:01:30Z

Qodana for .NET

49 new problems were found

Inspection name	Severity	Problems
`Auto-property accessor is never used (private accessibility)`	🔶 Warning	19
`Non-accessed field (private accessibility)`	🔶 Warning	13
`Possible 'System.NullReferenceException'`	🔶 Warning	4
`Redundant type declaration body`	🔶 Warning	3
`Disposal of a variable already captured by the 'using' statement`	🔶 Warning	2
`Parameter hides member`	🔶 Warning	2
`Get-only auto-property is never assigned`	🔶 Warning	2
`Member hides static member from outer class`	🔶 Warning	1
`Return value of a method annotated with [MustDisposeResource] is never disposed`	🔶 Warning	1
`Redundant using directive`	🔶 Warning	1
`Add/remove 'this.' qualifier`	◽️ Notice	1

💡 Qodana analysis was run in the pull request mode: only the changed files were checked
☁️ View the detailed Qodana report

Contact Qodana team

Contact us at qodana-support@jetbrains.com

Or via our issue tracker: https://jb.gg/qodana-issue
Or share your feedback: https://jb.gg/qodana-discussions

dennisdoomen force-pushed the Spike/RevisitAssertionScopeTake2 branch 3 times, most recently from bec8dea to a517fec Compare January 6, 2024 16:13

SAVEPOINT

2f85480

dennisdoomen force-pushed the Spike/RevisitAssertionScopeTake2 branch from a517fec to 2f85480 Compare January 6, 2024 21:07

dennisdoomen changed the title ~~Spike: Revisit AssertionScope and how it travels over chained calls~~ Redesign AssertionScope and how it travels over chained calls Jan 14, 2024

dennisdoomen changed the title ~~Redesign AssertionScope and how it travels over chained calls~~ Spike: Redesign AssertionScope and how it travels over chained calls Jan 14, 2024

dennisdoomen changed the title ~~Spike: Redesign AssertionScope and how it travels over chained calls~~ Redesign AssertionScope and how it travels over chained calls Jan 21, 2024

SAVEPOINT

549f752

dennisdoomen force-pushed the Spike/RevisitAssertionScopeTake2 branch 2 times, most recently from fe5cb45 to 17fbaa6 Compare April 6, 2024 15:50

dennisdoomen force-pushed the Spike/RevisitAssertionScopeTake2 branch 2 times, most recently from adaec55 to 6d7b28b Compare April 23, 2024 17:46

dennisdoomen force-pushed the Spike/RevisitAssertionScopeTake2 branch from 6d7b28b to 037a377 Compare April 27, 2024 17:36

dennisdoomen force-pushed the Spike/RevisitAssertionScopeTake2 branch 3 times, most recently from be16e9a to 3118f0b Compare April 29, 2024 17:42

SAVEPOINT

fedcaa8

dennisdoomen force-pushed the Spike/RevisitAssertionScopeTake2 branch from 3118f0b to fedcaa8 Compare May 4, 2024 11:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redesign AssertionScope and how it travels over chained calls #2539

Redesign AssertionScope and how it travels over chained calls #2539

dennisdoomen commented Jan 6, 2024 •

edited

github-actions bot commented Jan 6, 2024 •

edited

jnyrup commented Jan 8, 2024

dennisdoomen commented Jan 8, 2024

dennisdoomen commented Feb 25, 2024 •

edited

IT-VBFK commented Feb 25, 2024

jnyrup commented Feb 25, 2024

codymullins commented Feb 25, 2024 •

edited

obiwanjacobi commented Feb 25, 2024

jscarle commented Feb 25, 2024

dennisdoomen commented Feb 27, 2024 •

edited

jscarle commented Feb 27, 2024

ITaluone commented Feb 27, 2024

vbreuss commented Feb 27, 2024

victorvogelpoel commented Mar 1, 2024

dennisdoomen commented Mar 2, 2024 •

edited

github-actions bot commented Mar 31, 2024 •

edited

Redesign AssertionScope and how it travels over chained calls #2539

Are you sure you want to change the base?

Redesign AssertionScope and how it travels over chained calls #2539

Conversation

dennisdoomen commented Jan 6, 2024 • edited

IMPORTANT

github-actions bot commented Jan 6, 2024 • edited

Qodana for .NET

jnyrup commented Jan 8, 2024

dennisdoomen commented Jan 8, 2024

dennisdoomen commented Feb 25, 2024 • edited

IT-VBFK commented Feb 25, 2024

jnyrup commented Feb 25, 2024

codymullins commented Feb 25, 2024 • edited

obiwanjacobi commented Feb 25, 2024

jscarle commented Feb 25, 2024

dennisdoomen commented Feb 27, 2024 • edited

jscarle commented Feb 27, 2024

ITaluone commented Feb 27, 2024

vbreuss commented Feb 27, 2024

victorvogelpoel commented Mar 1, 2024

dennisdoomen commented Mar 2, 2024 • edited

github-actions bot commented Mar 31, 2024 • edited

Qodana for .NET

dennisdoomen commented Jan 6, 2024 •

edited

github-actions bot commented Jan 6, 2024 •

edited

dennisdoomen commented Feb 25, 2024 •

edited

codymullins commented Feb 25, 2024 •

edited

dennisdoomen commented Feb 27, 2024 •

edited

dennisdoomen commented Mar 2, 2024 •

edited

github-actions bot commented Mar 31, 2024 •

edited