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
Refactor approval tests #1669
Refactor approval tests #1669
Conversation
By default ApprovalTests uses the name of the test to name the approved API file, but now all TFMs share the same name, so I had to customize the file names. I thought it was a good opportunity to reorganize those files. Let me know if you don't like it. |
[UseReporter(typeof(DiffReporter))] | ||
[MethodImpl(MethodImplOptions.NoInlining)] | ||
public void ApproveApi45() | ||
public void ApproveFakeItEasyApi(string frameworkVersion) |
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.
When I started reading, I was expecting to see a test parameterized by project name and frameworkVersion. This is good, though!
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.
I was expecting to see a test parameterized by project name and frameworkVersion
It could. Would you prefer it that way?
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.
I pushed a new commit implementing that change
5d357a6
to
bd30718
Compare
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.
I was just making a comment about my guesses. I hadn't meant to push you into making the change—I was actually neutral on it. Until I saw the change, and I like the new way better, so I'm merging.
Thanks!
Thanks for the merge! Next in line, #1662, which I just rebased. I'm afraid I have a few more coming after that... |
This change has been released as part of FakeItEasy 5.4.0. |
As discussed in #1666