-
Notifications
You must be signed in to change notification settings - Fork 746
Add the ability to use params arguments properly when using a parametrised test fixture #4478
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @Shiney for your contribution.
I only have a few small comments.
src/NUnitFramework/tests/Attributes/ParameterizedTestFixtureTests.cs
Outdated
Show resolved
Hide resolved
Hi @manfred-brands I tried to do your comments about swapping the order of the checks but it didn't seem to end up making the code much nicer in my opinion because it ended up having to do all sorts of extra special code to deal with the case where zero parameters provided were used for the params argument |
@Shiney I tried to push my review changes, but you didn't allow maintainers to update the fork. |
Yes will do this soon |
I believe I've fixed all the comments and have ticked the Allow edits by maintainers button just in case |
…s edge cases not covered
Co-authored-by: Manfred Brands <manfred-brands@users.noreply.github.com>
7efb8b0
to
fc98d9d
Compare
Thanks @Shiney. I have fixed some spacing issues as earlier this week we enabled enforcing these rules. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nothing further.
Added some tests for this and made the methods that call the constructors on TestFixtures able to allow params arguments. This doesn't really address the duplication with similar code for calling test methods but that didn't seem particularly quick to also fix as part of this.
Fixes #1459