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

Feature request: Assert.Multiple() could return an IDisposable, avoiding passing an Action around. #4587

Open
andrewimcclement opened this issue Dec 13, 2023 · 10 comments

Comments

@andrewimcclement
Copy link
Contributor

andrewimcclement commented Dec 13, 2023

Would it be possible to be able to write code like:

// Act
var result = GetResult();

// Assert
using (Assert.Multiple())
{
    Assert.That(result.A, Is.EqualTo("A"));
    Assert.That(result.B, Is.EqualTo("B"));
}

This would hopefully avoid capturing unnecessary context into the Action (because it wouldn't exist). Async code becomes slightly simpler (no need to specify async again) also.

It might also make things like #4242 easier to get right.

Considerations:

  • As written, this would not conflict, but we would certainly want to drive users towards this as a preferred option over the existing Assert.Multiple(() => { ... }); option. So we could consider using a different name to avoid ambiguity.
@OsirisTerje
Copy link
Member

This would be a syntax like FluentAssertions AssertionScope.

@andrewimcclement Can you elaborate on what the concern is about capturing context in the Action?

If implemented, it has to be a another method, could use the same word AssertionScope, so that we don't introduce a breaking change.

For me, I find the existing syntax easy, and this more confusing. Further, will these not trigger the IDE0063 (Simplify using statement). If it does, and the user inadvertency accept it, it will cover all asserts to the end of the test. Not exactly what one would normally want.

@jnm2 You're on the team - and upvoted this. Can you comment on your thoughts about it?

@manfred-brands
Copy link
Member

Although I like the idea, there will be people that would call this statement without a using at all. That would mean the multiple is not reset. Unless nunit does that at the end of each test.
The current syntax has no such issue.

@OsirisTerje
Copy link
Member

OsirisTerje commented Dec 13, 2023

@manfred-brands I absolutely do agree.

But something like:

using (new AssertionsScope())
{
.....
}

would solve that I think.

But again: Why do you feel this is a better syntax? (Curious)

PS: Passing the Action around is happening internally in NUnit. I don't think the user would care. Do we gain anything else here?

@jnm2
Copy link
Contributor

jnm2 commented Dec 13, 2023

If implemented, it has to be a another method, could use the same word AssertionScope, so that we don't introduce a breaking change.

I don't see why it has to be another method, since we could have void Multiple(Action) and IDisposable Multiple() overloads, but I agree that the word Scope should be included so that the API is less confusing.

Although I like the idea, there will be people that would call this statement without a using at all. That would mean the multiple is not reset.

NUnit analyzers can help.

But again: Why do you feel this is a better syntax? (Curious)

  • It's automatically async-friendly.
  • Tools can analyze variable usage better between the inside and outside of the block. Things like definite assignment of variables will work, and nullability flow analysis.
  • It's a familiar style of API for me, and it's the choice I go with any time I have the choice (versus taking a delegate).
  • (least important by far to me) It's lighter-weight from a performance perspective, versus a lambda capturing class.

@OsirisTerje
Copy link
Member

OsirisTerje commented Dec 13, 2023

Ok, got it.

So, should it be Assert.MultipleScope, which would probably be the same as new MultipleScope(),
or just overload the existing Multiple ,
or should it be something along AssertionScope ?

Or something else? Suggestions @nunit/framework-team ? @andrewimcclement And please, if you like this idea/feature, upvote it even if you have no comments.

@andrewimcclement
Copy link
Contributor Author

Disabling IDE0063 via NUnit.Analyzers would likely be a good complement to this feature.
I don't mind about the terminology, though I think MultipleScope isn't necessarily ideal - it doesn't convey that it is related to assertions. AssertionScope seems fine. If we wanted a method on Assert that wasn't Assert.Multiple(), maybe Assert.Scope() would work?

@jnm2
Copy link
Contributor

jnm2 commented Dec 14, 2023

.NET 9 is adding a new API, System.Threading.Lock, with a method called EnterLockScope. What do you think about Assert.EnterScope() or Assert.EnterMultipleScope()?

@manfred-brands
Copy link
Member

I don't even think IDE0063 will come up as no variable is assigned, unless we think we need to return a Test multiple context or so. In that case we can indeed look at the analyzer to suppress it.

I do like EnterMultipleScope as it indicates better. EnterScope only raises questions about what kind of scope.

@stevenaw
Copy link
Member

stevenaw commented Dec 14, 2023

While I like the idea of naming an "AssertionScope" without the "multiple" identifier to encourage more arbitrary use but I don't have a strong preference. Someone may indeed want to use it for multiple asserts, or someone may instead want to use it for some isolated or scoped setup of a single assert in a larger method.

I do think that, whatever the name, semantically it should behave as a multiple assert block - which I suppose then speaks to @manfred-brands point about their preference to include Multiple in the name.

I suppose a third middle-ground here could be something like below. Debatably then, RunAll may or may not be the default.
EDIT: Though perhaps unneccessary? Open to feedback

public static IDisposable EnterScope(ScopeBehaviour behaviour);

public enum ScopeBehaviour {
  RunAll,
  StopOnFirstFailure
}

@stevenaw
Copy link
Member

Although I like the idea, there will be people that would call this statement without a using at all. That would mean the multiple is not reset.

NUnit analyzers can help.

Agreed re: analyzers.

Should the framework still track these internally to dispose of at the end of a test? If so, I think we should ensure we can handle nested scopes and define if they must be disposed in order and what should happen if a parent is disposed before it's child by a user. Would such a case automatically dispose the child or throw an error?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants