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.

Sign up for GitHub

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

Jump to bottom

RepeatAttribute increments CurrentRepeatCount on each repeat iteration #3696

Merged
merged 7 commits into from Dec 28, 2020
Merged

RepeatAttribute increments CurrentRepeatCount on each repeat iteration #3696

merged 7 commits into from Dec 28, 2020

Conversation

Antash
Copy link
Contributor

@Antash Antash commented Dec 23, 2020 •
edited

Closes #3662
RepeatAttribute should be incrementing the test context CurrentRepeatCount property for the sake of consistency.

@dnfadmin
Copy link

dnfadmin commented Dec 23, 2020 •
edited

CLA assistant check
All CLA requirements met.

@Antash Antash mentioned this pull request Dec 23, 2020
Closed
@Antash Antash marked this pull request as ready for review December 23, 2020 12:01
@Antash
Copy link
Contributor Author

Antash commented Dec 23, 2020

There is an CI issue although all tests are green.

@mikkelbu
Copy link
Member

@Antash I've just retriggered builds on master and there we also fail with the same error.

Some googling indicates that setting GenerateErrorForMissingTargetingPacks to false would correct this, see dotnet/runtime#45755, or perhaps setting DisableImplicitFrameworkReferences to true, but I need to examine this in more detail. I'm unsure how this could start to fail without any changes on master, but I guess it must be a change in the CI or the packages that we depend on (or recursively depend on).

@Antash
Copy link
Contributor Author

Antash commented Dec 27, 2020

@mikkelbu thank you for the investigation! Hope someone from the team will fix this soon, don't feel confident enough to change anything there on my own.

@mikkelbu mikkelbu mentioned this pull request Dec 27, 2020
Closed
mikkelbu
mikkelbu reviewed Dec 27, 2020
View reviewed changes
Copy link
Member

@mikkelbu mikkelbu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have PR that should solve the CI problem (even though I still not sure exactly what the problem is - which is very annoying).

Regarding this PR then I've left some comments, but the implementation seems sound. Besides this I also think we should update nunit/src/NUnitFramework/framework/TestContext.cs as it currently contains a TODO and some comments which are no longer correct (I don't know why the initial implementation left this part undone given the property name "CurrentRepeatCount" - but I cannot find any mention of the TODO in neither the issue #2647 nor the resolving PR #2648)

nunit/src/NUnitFramework/framework/TestContext.cs

Lines 173 to 181 in ad49f27

/// <summary>
/// Get the number of times the current Test has been repeated. This is currently only
/// set when using the <see cref="RetryAttribute"/>.
/// TODO: add this to the RepeatAttribute as well
/// </summary>
public int CurrentRepeatCount
{
get { return _testExecutionContext.CurrentRepeatCount; }
}

src/NUnitFramework/tests/Attributes/RepeatAttributeTests.cs Outdated
public void RepeatUpdatesCurrentRepeatCountPropertyOnEachAttempt()
{
RepeatingTestsFixtureBase fixture = (RepeatingTestsFixtureBase)Reflect.Construct(typeof(RepeatedTestVerifyAttempt));
ITestResult result = TestBuilder.RunTestCase(fixture, "PassesTwoTimes");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ITestResult result = TestBuilder.RunTestCase(fixture, "PassesTwoTimes");
ITestResult result = TestBuilder.RunTestCase(fixture, nameof(RepeatedTestVerifyAttempt.PassesTwoTimes));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. No Idea why I decided to ignore nameof. May be I was looking into existing RetryAttributeTests which uses plane strings either.

src/NUnitFramework/tests/Attributes/RepeatAttributeTests.cs Outdated
public void RepeatUpdatesCurrentRepeatCountPropertyOnGreenTest()
{
RepeatingTestsFixtureBase fixture = (RepeatingTestsFixtureBase)Reflect.Construct(typeof(RepeatedTestVerifyAttempt));
ITestResult result = TestBuilder.RunTestCase(fixture, "AlwaysPasses");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ITestResult result = TestBuilder.RunTestCase(fixture, "AlwaysPasses");
ITestResult result = TestBuilder.RunTestCase(fixture, nameof(RepeatedTestVerifyAttempt.AlwaysPasses));

@Antash Antash requested a review from mikkelbu December 28, 2020 08:46
@rprouse
Copy link
Member

rprouse commented Dec 28, 2020

@Antash like the other PR, I've submitted a PR to your fork to fix the build. Antash#2

rprouse
rprouse approved these changes Dec 28, 2020
View reviewed changes
Copy link
Member

@rprouse rprouse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We just need to get the build passing. Thanks.

@Antash
Copy link
Contributor Author

Antash commented Dec 28, 2020

@rprouse thank you! All green now.

@rprouse rprouse merged commit 9590876 into nunit:master Dec 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rprouse rprouse rprouse approved these changes

@mikkelbu mikkelbu Awaiting requested review from mikkelbu

Assignees
No one assigned
Labels
awaiting:contributor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

TestContext.CurrentContext.CurrentRepeatCount only contains retry count not the repeat count
4 participants
@Antash @dnfadmin @mikkelbu @rprouse