Repository navigation
NUnit 4.0 fails when [Repeat] is present and test passes #4491
Description
Activity
Thanks @RenderMichael ! This is really weird. I do get the same result.
The repro is here: https://github.com/nunit/nunit.issues/tree/main/Issue4491
It is the Repeat attribute that seems to cause this.
I added an ordinary test (see repro), and enabled dumping of the xml, and it looks like the Repeat attribute changes the NUnit ID.
That is not good.This is the two tests:
Test2 is the one with the repeat attribute, and have the Id = 0-1001. Test3 is the ordinary test, with the Id = 0-1002.
From the NUnitTestEvents, we get these two for the test cases:
Note that Test2 now have the Id = 0-1037, whereas Test3 still have the Id = 0-1002.
So, it looks like the Repeat command somehow sets a new number, an Id that doesn't exist in the discovery at all.
Adding writing out the test id to the output, the following is displayed (changed the repeat to 5),
So, the repeat attribute increments the Id, starting from the highest current one, which was 1007.
And in RepeatAttribute.cs, class RepeatedTestCommand,
The Id is updated. If this also had happened for discovery, it would have been better, but it would still fail when RTD is used in VS TE.
Note, that if it fails, it is not iterated, and thus keeps the original id.
Just double-checked whether
[Theory]has the same issue, since there ID is changed there as well. That works fine.You are right the individual tests in a theory have their own ID, but they are then children of the top node. And they also appear in the discovery with their correct number.
The idea with the Repeat was probably the same, and that makes sense, but it is not implemented that way.
I've added a Theory test to the repro, and the runsettings there have the Dump enabled for both discovery and execution, so it is possible to see what comes out from the framework.
It would be really nice if, as part of this fix, [Repeat] tests could appear in Test Explorer the same way as [Theory] tests, with children nodes.
However, that may be hard since repeat has a variable number of children, depending on when/if a test fails. I doubt changing [Repeat]'s behavior to always run, even on failure, is desirable.
However, [Repeat] worked in latest v3 so if it could be working again, that would be great enough for me.
@RenderMichael Since you've already been into the code, would you mind having a look at what has changed between v3 and v4 here?
@OsirisTerje I spent a bit of time looking into this and I can't pinpoint why or even when it started failing. The attribute itself hasn't changed in years.
All I managed to figure out is, this has been failing for a while. A couple years, if I haven't made any crucial mistakes.
@RenderMichael Ok, so it doesnt work in 3.13.3 either ?
It works in 3.13.3, but NUnit transitioned to v4 in February 2021 and it broke recently after that
@nunit/framework-team This issue broke in this release, worked in 3.13.3, no apparent changes, but could be a side effect of some other change that was done early 2021.
It would be nice to have this fixed, even if it is an attribute that is not much used. It may be other similar attributes having the same fault though.
It also passed all tests, meaning we're lacking a test for this case.We can fix it by deleting the SetNextId call mentioned above, but it should have been a parent/child type of test.
If we can't fix this before final release, it has to go on a known-bugs list.
@OsirisTerje The
SetNextIdcall you point at was added by @asos-tomlonghurst and also affectsRetry
It doesn't reference an issue and I can't even find an issue nor pr assigned to him.
It was his only contribution and I don't know what it was addressing.Within #3974 ? Good sleuthing @manfred-brands
That PR explicitly mentions that it aims to create new IDs for repeated runs which seems contrary to how it needs to work based on what we've discovered here.@asos-tomlonghurst can you explain a bit about what you're trying to solve. I'm curious if we can help suggest alternatives to having this id change between runs
@rprouse You reviewed that PR, can you remember what it was for?
I don't remember more than what is in the PR, sorry.
So that PR ensured unique-id, but nobody is expecting that and it breaks other tools/parsers.
Should we then revert that PR?
Is there any other way a consumer can find out that a test needed repeating?I'm ok with reverting it since it hasn't been included in a stable release yet - though I'd suggest we also revert it from the 3.x branch to avoid it accidentally going out there too. Does anyone else in @nunit/framework-team feel differently?
We can also help the contributor of that PR look for alternative ways of supporting their use case once we understand why they wanted unique IDs here
I agree that we should revert it (especially since we are unsure of what this solves and it blocks the release). Then we can always improve on this later on.
If we revert it, then it would be nice with also a test that captures the expected behaviour (before #3974)
Also agree we revert it, but as @mikkelbu says, nice to get a good test in there.
Since the PR has no linked issue, we can't reopen that non-existing issue.
@thomhurst If this is still an issue for you, can you please open an issue with the details, and we can take your situation from there?Sorry guys. I wasn't aware changing that ID would break things.
I had a test framework that was collecting test results for a report, and using that ID as a unique key for every different test/execution. If I had a Repeat(5), I still wanted to collect the 5 separate runs to report on. Because this id didn't change, results were being overwritten and only one stored.
If it's breaking things then definitely revert it and I apologize. I just needed a unique identifier and I thought that's what the purpose of that property was.
@thomhurst A bit out of the head, haven't checked this properly, but just throwing it out: I wonder if this setting in the adapter could help here: https://docs.nunit.org/articles/vs-test-adapter/Tips-And-Tricks.html#newoutputxmlfileforeachrun Setting it would create a separate test result file for each test run. That is, if the Repeat/Retry attributes actually produces separate outputs.
UPDATE:
No, this doesnt work here, since the Repeat attribute work on too low level.I wasn't even using the xmls, I was grabbing it during execution via the TestContext.Current.Test.Id (or whatever it is, I'm on mobile) property. Maybe a second property could be useful that's unique for every execution. Just thinking out loud
The second property is there, and is actually
TestContext.CurrentContext.CurrentRepeatCount.If one of the repeated runs fails, that will stop subsequent runs. The results for all repeated green runs would be exactly the same. Is what you're after to see the difference between the pre-repetitions that are green and a possibly failed ending repetition?
Or is it to see some indications of how many runs were run?
So, I think the question is: what is the use of the Ids, what are you trying to get out from them?It is notexactly that the results are being overridden. If you e.g. do a TestContext.Writeline like:
[Test] [Repeat(3)] public void Test1() { TestContext.WriteLine("Test1"); Assert.Pass(); }
The output results would be:
If it fails, like in this code:
[Test] [Repeat(6)] public void Test1() { if (TestContext.CurrentContext.CurrentRepeatCount == 3) { Assert.Fail(); } TestContext.WriteLine($"Test1 repeatcount: {TestContext.CurrentContext.CurrentRepeatCount}"); Assert.Pass(); }
The output would be:
So the output is being accumulated.
The duration is also equal to the total run, that is TimeInTest*RepeatcountCould you use something from this?
Trying out the new v4 preview on my test suites, I got the following issue
This passing test does not appear in the test results:
Running
dotnet testyieldswhereas the test
yields