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
Ensures expectations involving multiple assertions can be cleared #925
Ensures expectations involving multiple assertions can be cleared #925
Conversation
e99ff63
to
37cc1bb
Compare
37cc1bb
to
c54c008
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what the functional consequences are?
/// Indicates that every argument passed into <see cref="FailWith"/> is displayed on a separate line. | ||
/// </summary> | ||
public AssertionScope UsingLineBreaks | ||
public IAssertionScope UsingLineBreaks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤔 As IAssertionScope
doesn't have all the public members of AssertionScope
this is a breaking change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure. The members of IAssertionScope
are only used in chaining scenarios. I've included a couple of them, but not all. Any strong opinions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't tried to invoke AssertionScope
-only members myself, I just looked for changes that could break the public API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normally, you're directly using the AssertionScope
in a using
block. The interface is only used for those complex chaining purposes. In the last commit, I've added almost all of them to the interface, just to be sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But no, I don't have any strong opinions, except trying to avoid breaking changes.
Just as a note: the remaining differences between AssertionScope
and IAssertionScope
are:
public string Context { get; set; }
public void AddPreFormattedFailure(string formattedFailureMessage);
void AddNonReportable(string key, object value);
void AddReportable(string key, string value);
T Get<T>(string key);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I know. These are very specific things, mostly only used by BeEquivalentTo
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So? Approved?
c54c008
to
9356b83
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would these changes benefit from any tests exercising the code?
I'm in doubt. I would have to check whether the failure doesn't contain anything else. Not sure if it's worth though. I'll check when I continue with #911. |
Introduces a
ClearExpectation
to undo the expectation that is associated with multiple assertions usingWithExpectation
. Without it, it is possible that this expectation gets appended to any successive assertions.Fixes #918