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

Revisit AssertionScope and how it travels over chained calls #2253

Open
dennisdoomen opened this issue Aug 13, 2023 · 5 comments · May be fixed by #2539
Open

Revisit AssertionScope and how it travels over chained calls #2253

dennisdoomen opened this issue Aug 13, 2023 · 5 comments · May be fixed by #2539
Assignees
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation breaking change
Milestone

Comments

@dennisdoomen
Copy link
Member

dennisdoomen commented Aug 13, 2023

Although I'm still thinking about the design, I propose the following changes:

  • 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.
  • Every Should method will also take an extra parameter annotated with [CallerArgumentExpression]. If this returns a non-null value, we can use that instead of using the CallerIdentifier.
  • 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.
  • This should allow methods like ContainSingle to update the Assertion instance with extra information to solve Misleading caller identity when Which fails #1502
  • 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
@dennisdoomen dennisdoomen added breaking change api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Aug 13, 2023
@dennisdoomen dennisdoomen added this to the 7.0 milestone Aug 13, 2023
@Corniel
Copy link
Contributor

Corniel commented Aug 15, 2023

Do you have some (pseudo) code in mind on how the body of a certain assert method should look like?

@dennisdoomen
Copy link
Member Author

I'm going to do a POC soon to proof this idea to myself ;-)

@dennisdoomen dennisdoomen self-assigned this Aug 27, 2023
@jnyrup
Copy link
Member

jnyrup commented Sep 9, 2023

Perhaps related to this idea:
Use [CallerArgumentExpression] to retrieve the name of the subject at compile-time and populate this Assertion object with that.
The idea is to remove the need for CallerIdentifier, which is both slow and fragile.

@dennisdoomen
Copy link
Member Author

Yeah, that could work indeed. And in case it doesn't work (.NET 6 SDK not installed and using .NET Framework 4.7), we can fall back to the CallerIdentifier.

@dennisdoomen
Copy link
Member Author

The first attempt where Which would return a WhichResult that includes both the result as well as the current assertion failed for a couple of reasons. But sometimes we directly call methods or access properties on the result of Which, which means the type of what Which returns must be TElement. So trying to pass a compound object from one Should to another through a special AndWhichConstraint was more difficult than I thought.

@dennisdoomen dennisdoomen linked a pull request Jan 6, 2024 that will close this issue
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation breaking change
Projects
Status: 🏗 In progress
Development

Successfully merging a pull request may close this issue.

3 participants