Implement Fake.ClearRecordedCalls #979
Conversation
|
Oh, thanks, @erik-kallen. I've invited you to be a collaborator. This would allow us to assign the issue to you. |
There was a problem hiding this comment.
Thanks, @erik-kallen. It generally looks very good. I just have one question and a request for a small change to the tests.
| ICompletedFakeObjectCall ignored; | ||
| while (this.recordedCalls.TryDequeue(out ignored)) | ||
| { | ||
| } |
There was a problem hiding this comment.
I don't think it's a big deal, but how do you feel about just replacing the queue with a new one?
There was a problem hiding this comment.
Agreed. Replacing the queue with a new one is atomic, whereas dequeueing all items is not.
|
|
||
| // Assert | ||
| manager.GetRecordedCalls().Should().BeEmpty(); | ||
| } |
There was a problem hiding this comment.
The new tests are great, and I think you've added them here because you found places we were testing Fake. Could I ask a favour, though?
We've been moving toward putting as many tests as is practical in the Specs, with even more of a focus on this when we're talking about new or changed behaviour.
How would you feel about replacing this test to the Acceptance tests? The FakeSpecs class looks to be a good place.
Then FakeFacadeTests.ClearRecordedCalls_should_call_clear_recorded_calls_on_manager could be struck.
|
Hey, @erik-kallen. It's been nearly 3 weeks since we've heard from you. Are you interested in pursuing this pull request any further? We'd be happy to provide guidance, if anything's confusing. |
|
It would probably be easier for a maintainer to implement the requested changes than to review the changes after I make them. Especially moving the test seems like I will put it in the wrong place and have to redo it again. |
|
@erik-kallen, thanks for the speedy response. If you're worried about the amount of work for us, please do not. Even if you're right, we're happy to spend the time working with contributors. If you're worried about the amount of work for you, then that's a different story, and I respect your position. However, I'm confident that you'd have no problems implementing the changes. If it helps, the new specs could be added right at the bottom of FakeSpecs.cs, and the existing All that being said, if you prefer not to continue, that's perfectly fine. Just say the word and one of us will put the finishing touches on the PR. |
Implement
Fake.ClearRecordedCalls.Fixes #978.