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
Provide a way to ignore a failed assertion #4701
Comments
@maettu-this Can you explain the reasoning for this, i.e. What is the practical use-case? |
@manfred-brands, sure: From TestCentric/testcentric-gui#1049:
More detailed from the origin:
I cannot circumvent or even fix these issues, as they are given 3rd party hardware or its drivers. #2758 and #3245 indicate other users and use cases face the same issue. Summarizing more generally phrased: There are cases where test logic is called, but under certain conditions is accepted to fail. |
Another use case is related to local TCP/IP and UDP/IP servers. When running tests in parallel, the system may assign the same local port number to multiple servers. Only one will successfully be able to bind the socket to the port, the others will fail and have to retry on another local port. Pseudo code:
This use case I currently work around by explicitly implementing the assertion rather than using
This change enables migration with minimal impact on the test implementation. |
I see that you might have to try something a few times, but I don't see why that needs to throw an exception simply to try again. Exceptions are expensive and should not be used for this case. As Nich Chapsas says: Exceptions should be thrown when something exceptional happens, not for flow control. See https://www.youtube.com/watch?v=a1ye9eGTB98 private void StartServer()
{
for (int i = 0; i < MaxRetries; i++)
{
if (server.Start())
return;
server.LocalPort = GetAvailablePortFromSystem(); // try again
}
Assert.Fail($"Failed to start server after {MaxRetries} retries.");
} I know that this was an example, but it would be much better that the Alternatively you could use the |
For the For the hardware reliablity case I think exceptions are OK to be used, as the issue happens in about 1 out of a thousand times, i.e. indeed is exceptional. Of course I could redesign the test in a way to handle this in a different way, but why making the test implementation less obvious by no longer using These are two concrete examples where the pre 3.10 ability to ignore/reset a failed test result was convenient and pragmatic. Of course there are other ways to implement this, but why make things more complicated if they can be simple? I don't know the details behind #2758 and #3245 but apparently others have been using this simple approach as well. |
I looks like the below works for me: [Test]
public void ResetsPreviousErrors([Values] bool value)
{
try
{
Assert.That(value, Is.True);
}
catch (AssertionException)
{
NUnit.Framework.Internal.TestExecutionContext.CurrentContext.CurrentResult.AssertionResults.Clear();
}
} We might want to add a convenience method like |
I think the idea of allowing more control over this state or scope could be helpful. There's a few other similar discussions over time where some assumptions about pre-3.10 behaviors didn't align with the #3337 has a bit of background, in particular about past guidance not to rely on thrown exceptions while #4587 outlines another option to "scope" assertions. @manfred-brands I feel like we've looked at ideas for a @nunit/framework-team is this something we should add? |
Oh! It was #4461 EDIT: Seems the nature of the issue was slightly different than I remember, but perhaps seeking the same motivation of exposing more control over flow within a test containing many assertions |
Excellent @manfred-brands, this looks very promising! Basic verification looks good, full verification in my test setup I haven't managed to complete today. I should have this done by Tuesday. Of course a convenience method would nice, rather than having to call such internal method. To me, having embedded systems backgound,
I haven't yet fully understood the difference among My personal favorite is In any case, previously linked #2758 and #3245 should also be happy with what will be concluded here. |
@maettu-this Thanks for the great naming ideas. I admit, I'm on the fence myself about adding this. Exposing a formal API to control NUnit state directly like this takes some "guard rails" off, and any new feature always carries a potential to make others more difficult to evolve if they're found to be in conflict with each other. However, there also seem to have been a few pain points over time that I thought the idea could be a good starting point for discussion. 🙂 |
@manfred-brands, I confirm that I can cover all my use cases with...
...and @stevenaw, myself I can live with using the internals, I have created a However, while I fully understand the trade-off for long term maintainability, still, the several issues indicate this is a demand. I think NUnit should come up with a non-internal solution. Providing a public wrapper method should decouple the public API well-enough from the internals, while having the public API will make sure the demand for such functionality is kept covered. |
At the time this change was made, we had been telling people for years "Don't rely on catching the exception because NUnit's use of exceptions is considered an internal implementation detail, which may change at any time. When we changed it for a major release, some folks complained, of course. One problem was a third-party runner, which relied on the ability to catch the exceptions. IIRC we added TestExecutionContext.IsolatedContext for use by that runner and similar situations, since TestContext was user-facing while TestExecutionContext was part of our internal implementation. The idea was that anyone using internals should know they are internals and accept the risk. My 2 cents on this is that behavior where a failing test "changes its mind" and decides it should pass is not something to encourage. Making it hard to do could be considered a feature. |
Thanks @CharliePoole that's a good point and perspective about tests which may "change their minds". @manfred-brands I know the suggestion was one originally raised by you here which I'd then run with a bit. What are your thoughts on it all? @maettu-this Great to hear as well that you've been able to adjust things on your side to make things work independent of any action taken here :) |
@stevenaw, well, I was able by making use of NUnit internals. It works, but I as previously mentioned I still think this is something that should be publicly available. @CharliePoole, I do agree that changing the tests is preferred, but in the majority of cases I face it would have made the test logic way more complex than necessary. So I stick with "why make things more complicated if they can be simple..." adding "..and obvious". Tests "changing their mind" enables reusing existing test code even for cases where things occasionally go awry, for known or even good reasons. I think reusing such code is preferred over making things more complex. |
Refined here from TestCentric/testcentric-gui#1049.
Pre NUnit 3.10 it was possible to catch an
AssertionException
to ignore a failed assertion and still get aPassed
test. Upgrading from pre NUnit 3.10 requires to migrate test code that uses this technique to "something else" available with NUnit 3.10+.NUnit should provide some way to achieve this. My preference:
AssertionException
, probably unlikely.TestContext.CurrentContext.ResetResult()
.Assert.Pass
to "cancel" or "ignore" a previous failure as proposed by Should Assert.Pass() revert a failed test? #3245.TestExecutionContext.IsolatedContext
as discussed in Catching AssertionException fails tests since 3.10. #2758. However, that is a yet internal infrastructure.The text was updated successfully, but these errors were encountered: