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
Comments
This would be a syntax like FluentAssertions @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 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? |
Although I like the idea, there will be people that would call this statement without a |
@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? |
I don't see why it has to be another method, since we could have
NUnit analyzers can help.
|
Ok, got it. So, should it be Or something else? Suggestions @nunit/framework-team ? @andrewimcclement And please, if you like this idea/feature, upvote it even if you have no comments. |
Disabling IDE0063 via NUnit.Analyzers would likely be a good complement to this feature. |
.NET 9 is adding a new API, System.Threading.Lock, with a method called EnterLockScope. What do you think about |
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 |
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 I suppose a third middle-ground here could be something like below. Debatably then,
|
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? |
Would it be possible to be able to write code like:
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:
Assert.Multiple(() => { ... });
option. So we could consider using a different name to avoid ambiguity.The text was updated successfully, but these errors were encountered: