Skip to content

Add support for Theories in nested test classes #3857

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

Merged
merged 7 commits into from
Oct 2, 2021

Conversation

Crown0815
Copy link

fixes #3856

We already have a working solution for the issue described in #3856 which I adapted to the existing NUnit Theory attribute.
Please let me know what I can improve to make this a viable contribution.

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.

@nunit/framework-team I don't see a problem with adding this flag, as the addition is compatible with existing code. Do any of you have concerns?

@Crown0815 I think we should also add some data in https://github.com/nunit/nunit/blob/master/src/NUnitFramework/testdata/TheoryFixture.cs with a Theory(true) and a Theory attribute in the same class and then a test in src/NUnitFramework/tests/Attributes/TheoryTests.cs that tests that the boolean affects the search.

private readonly bool _searchInDeclaringTypes;

/// <summary>
/// Creates new DatapointProvider
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
/// Creates new DatapointProvider
/// Creates a new DatapointProvider.

/// <summary>
/// Creates new DatapointProvider
/// </summary>
/// <param name="searchInDeclaringTypes">Determines whether when searching for theory data members of declaring types will also be searched</param>
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
/// <param name="searchInDeclaringTypes">Determines whether when searching for theory data members of declaring types will also be searched</param>
/// <param name="searchInDeclaringTypes">Determines whether when searching for theory data members of declaring types will also be searched.</param>

@Crown0815
Copy link
Author

@mikkelbu I made the requested changes.

Apparently, some of the tests I added are failing, but I am not able to get them to run locally. Could you please explain what I have to do so I can run these tests locally. They are not discovered by VSTest.

@mikkelbu
Copy link
Member

Apparently, some of the tests I added are failing, but I am not able to get them to run locally. Could you please explain what I have to do so I can run these tests locally. They are not discovered by VSTest.

Currently, there are some issues running the tests in VS directly. There is a possible solution here
#3861 (reply in thread). Alternatively, you can run build --target=Test --configuration=Release from powershell to run our cake scripts - see https://github.com/nunit/nunit/blob/master/BUILDING.md.

The tests are failing as reflection is trying to access the field i0 on NestedWhileSearchingInDeclaringType. I guess we need to return a pair/tuple from GetMembersFromType - the Type and the MemberInfo, so we know which type has the MemberInfo. We should then use this type in all the places where we call ProviderCache.GetInstanceOf, so we use the right Type.

Without the owning type members of nested types will not be
resolved correctly when using `ProviderCache.GetInstanceOf(fixtureType)`
since the `fixtureType` does not have the member in question.
@Crown0815
Copy link
Author

Crown0815 commented Jul 6, 2021

@mikkelbu I could not get the local tests to run, but your proposed changes fixed the failing tests. I considered a specialized class to encapsulate some of the operations executed on the member but then went with the proposed Tuple since it was a quick fix.

Please let me know what you think.

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.

Looks good to me. Do you want to see any other changes @mikkelbu ?

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.

@Crown0815 I'm sorry. I completely lost track of this during the summer. I'll merge the changes now.

@mikkelbu mikkelbu merged commit bf4dd68 into nunit:master Oct 2, 2021
@mikkelbu mikkelbu added this to the 4.0 milestone Oct 2, 2021
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.

Theories in nested Testfixtures
3 participants