search

wiremock / wiremock-grpc-extension

WireMock Extension: gRPC mocking
https://wiremock.org/docs/grpc/
Apache License 2.0
15 stars 8 forks source link

Fixing reset to also clear counters #59

Closed edeandrea closed 6 months ago

edeandrea commented 7 months ago

When calling WireMockGrpcService.removeAllStubs() it would successfully remove the stubs but it would not reset the counts of any previously-called interactions. This needs to be addressed.

This PR addresses this issue and also adds another test to prove that it does what its supposed to do.

edeandrea commented 7 months ago

Not sure why the spotless is failing on windows. I ran ./gradlew :spotlessApply before I committed.

edeandrea commented 6 months ago

Hi @tomakehurst any chance you might get to review/merge this?

tomakehurst commented 6 months ago

Thanks for raising this @edeandrea, but this behaviour isn't consistent with WireMock's core.

Verification isn't dependent on stubs existing or not, just requests matching the criteria given so I don't think it's a good idea to merge this PR.

edeandrea commented 6 months ago

Thanks @tomakehurst I'm not sure I understand your comment. This PR doesn't have anything to do with verification. It fixes the fact that the reset behavior doesn't currently reset invocation counters. It brings the WireMockGrpcService.removeAllStubs() functionality consistent with WireMockServer.resetAll(). Right now it isn't.

tomakehurst commented 6 months ago

WireMock doesn't have any invocation counters, only events in the request journal. These are totally independent of stubs, so in WireMock core removing a stub (or all stubs) has no effect on the contents of the journal.

However, if we wanted to implement resetRequests() on a mock gRPC service then finding and removing just the events associated with that service would make sense.

edeandrea commented 6 months ago

Ok, so maybe what we need here then is a resetAll() method that matches the one in WireMockServer? Because thats what's really missing, and what I was really after when I did #27 (I even called it resetAll originally :) )

tomakehurst commented 6 months ago

Yes, it would also be valid to do this under resetAll(), although I would using the service part of the URL to filter the events for removal rather than matching stubs, that way if the user has removed a stub already we still clean up corresponding events.

edeandrea commented 6 months ago

Ok so maybe what I'll do is create a resetRequests() method where I will reset the events for this service, and then create a resetAll which just calls the existing removeAllStubs() and the new resetRequests().

Hows that sound?

tomakehurst commented 6 months ago

That sounds good to me 👍

edeandrea commented 6 months ago

Hi @tomakehurst I've gone ahead and re-worked this PR based on our conversation. I'm not sure why only the windows-11 build is failing due to formatting issues. I did run spotlessApply before committing anything (I'm on a mac though).

edeandrea commented 5 months ago

hi @tomakehurst I was curious if you might be doing a release at some point which includes this?