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

Add method to set Until date when ignoring TestCaseData #3277

Closed
wants to merge 6 commits into from
Closed

Add method to set Until date when ignoring TestCaseData #3277

wants to merge 6 commits into from

Conversation

abborg
Copy link
Contributor

@abborg abborg commented Jun 6, 2019 •
edited

Resolves #3275

Provide a simple method to set the IgnoreUntilDate on a test case defined by TestCaseData. The behavior is the same as IgnoreAttribute and only applies the ignored status if the test is not already set to NotRunnable and the Until date has not been reached yet.

@dnfclas
Copy link

dnfclas commented Jun 6, 2019 •
edited

CLA assistant check
All CLA requirements met.

mikkelbu
mikkelbu previously approved these changes Jun 7, 2019
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.

This looks good to me @abborg. I've only a couple of very minor comments.

Ps. The PR will need one additional review before it can be merged to master.

src/NUnitFramework/framework/TestCaseData.cs Outdated Show resolved Hide resolved
src/NUnitFramework/testdata/TestCaseSourceAttributeFixture.cs Outdated Show resolved Hide resolved
@abborg
Copy link
Contributor Author

abborg commented Jun 8, 2019

Thank you for reviewing the changes and providing feedback @mikkelbu! I'll make the amendments you noted.

CharliePoole
CharliePoole reviewed Jun 8, 2019
View reviewed changes
Copy link
Contributor

@CharliePoole CharliePoole left a comment

Choose a reason for hiding this comment

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

The implementation looks fine to me, but I am wondering whether this is the API we want. Just commenting for now, will enlarge on the issue itself, and review again when we agree on the API.

@rprouse
Copy link
Member

rprouse commented Jun 10, 2019

This looks good to me too, but I agree with @CharliePoole about discussing the API. I am fine with adding an overload with a second parameter as it matches the Ignore attribute. I am not sure if the second parameter should be a string though. The Ignore attribute uses string as a workaround because attribute parameters must be const, but we don't have the same limitation with TestCaseData.

I think the until parameter should be strongly typed as a DateTime. The advantage is that date format mistakes will be caught at compile time. The disadvantage is that constructing a new DateTime is more typing than a simple string. As it is, the code takes a string, tries to convert to a DateTime then converts back to a string in a known format. It also fails silently if the string parameter is in a bad format.

jnm2
jnm2 requested changes Sep 23, 2019
View reviewed changes
src/NUnitFramework/framework/Internal/IgnoredTestCaseData.cs
{
this.RunState = RunState.Ignored;
string reason = (string)this.Properties.Get(PropertyNames.SkipReason);
reason = string.Format("Ignoring until {0}. {1}", datetime.ToString("u"), reason);
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to deduplicate this code. I just realized that this is a copy of the code in IgnoreAttribute.

@nunit/framework-team Any thoughts on where we should keep common code like this?

Copy link
Member

Choose a reason for hiding this comment

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

I agree, but this PR has been out there for many months. Other than this, I think it looks good and can be merged. Maybe we leave the refactoring to another PR?

Copy link
Contributor

Choose a reason for hiding this comment

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

@rprouse That sounds good!

rprouse
rprouse previously approved these changes Sep 25, 2019
View reviewed changes
src/NUnitFramework/framework/Attributes/TestCaseAttribute.cs Show resolved Hide resolved
src/NUnitFramework/framework/Attributes/TestCaseAttribute.cs Show resolved Hide resolved
src/NUnitFramework/framework/Internal/IgnoredTestCaseData.cs
{
this.RunState = RunState.Ignored;
string reason = (string)this.Properties.Get(PropertyNames.SkipReason);
reason = string.Format("Ignoring until {0}. {1}", datetime.ToString("u"), reason);
Copy link
Member

Choose a reason for hiding this comment

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

I agree, but this PR has been out there for many months. Other than this, I think it looks good and can be merged. Maybe we leave the refactoring to another PR?

@rprouse
Copy link
Member

rprouse commented Oct 10, 2019

@jnm2 would you still like to see changes to this? If not, I'll resolve conflicts and merge. This has been out there too long. I apologize for that @abborg.

@rprouse rprouse self-assigned this Oct 10, 2019
@jnm2
Copy link
Contributor

jnm2 commented Mar 3, 2020

@rprouse The copy/paste is the only outstanding thing that bothers me. #3277 (comment)

@rprouse rprouse mentioned this pull request May 11, 2020
Merged
@rprouse
Copy link
Member

rprouse commented May 11, 2020

This PR has been superseded by #3544.

@rprouse rprouse closed this May 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CharliePoole CharliePoole CharliePoole left review comments

@oznetmaster oznetmaster oznetmaster left review comments

@jnm2 jnm2 jnm2 requested changes

@rprouse rprouse rprouse left review comments

@mikkelbu mikkelbu mikkelbu left review comments

Assignees

@rprouse rprouse

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enable setting IgnoreUntilDate in TestCaseData.Ignore
7 participants
@abborg @dnfclas @rprouse @jnm2 @mikkelbu @CharliePoole @oznetmaster