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.

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

Adds an analyzer for suggesting tests move to the strongly-typed TheoryData classes (xunit/xunit#1244) #171

Merged
merged 11 commits into from
Dec 4, 2023

Conversation

JamesTerwilliger
Copy link
Contributor

It appears that one of my previous checkins had some needed follow-on by the project organizer. My sincere apologies for the inconvenience. I've taken some effort in this PR to look for more edge cases when possible.

This PR is a proposed resolution to issue xunit/xunit#1244. It adds an analyzer to suggest someone who is not using TheoryData to do so for the benefit of strong typing.

In working through scenarios, it occurred to me that someone may have subclassed TheoryData. Therefore, I updated the logic to allow for subclassing and still detect properly. Test cases have been added to demonstrate such.

@bradwilson I hope this PR is better than the last one for quality.

@bradwilson
Copy link
Member

There are a couple things I want to change with this PR, which I'm going to do myself (so nothing required on your side).

  1. I don't want to recommend people away from IEnumerable<ITheoryDataRow> in v3 until/unless there's an equivalent to TheoryData<T>. Unlike IEnumerable<object[]> (and TheoryData<>) which is about just returning raw data, IEnumerable<ITheoryDataRow> is about returning data + metadata (like being able to add traits to individual data rows, skip individual data rows, etc.). This is a trivial fix.
  2. The changes to MemberDataShouldReferenceValidMemberTests are probably not exactly what I want (I'd rather shift to TheoryData<> than add xUnit1042 expectations to every test). But it also brings up a long overdue restructuring of this source file which needs to be done, because the tests for the many rules are sometimes intermingled, and occasionally duplicated, and it's hard to isolate where things should be. So I'm going to restructure this file, and in the process, make return value changes to all the non-xUnit1042 tests so that they won't orthogonally trigger that rule.

As for apologizing for missing edge cases...it's super easy to miss edge cases here, and I miss them all the time. 😄 I do have the advantage of having filed away 16 years of xUnit.net history in my head so that it might make it easier for me to recognize some of those edge cases (this is especially the case for v3-related stuff, since as near as I can tell, I'm the only person who's ever used v3 in any way 😂). Believe me, even submitting PRs with a few missed edge cases is super helpful and appreciated.

@bradwilson bradwilson changed the title Adds an analyzer for suggesting tests move to the strongly-typed TheoryData classes Adds an analyzer for suggesting tests move to the strongly-typed TheoryData classes (xunit/xunit#1244) Dec 4, 2023
@bradwilson bradwilson merged commit 2b4f69b into xunit:main Dec 4, 2023
5 checks passed
@bradwilson
Copy link
Member

Thanks!

@JamesTerwilliger
Copy link
Contributor Author

Much gratitude to you, @bradwilson . Thanks.

@JamesTerwilliger JamesTerwilliger deleted the jamest/theorydata branch December 4, 2023 02:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants