mirror of
https://github.com/wiremock/WireMock.Net.git
synced 2026-01-11 22:30:41 +01:00
Updating to 1.1.6+ breaks tests because new AllowAnyHttpStatusCodeInResponse option defaults to false #252
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Originally created by @eli-darkly on GitHub (Feb 15, 2020).
I have some tests that validate my client code's handling of various server error conditions. After updating WireMock.Net to the latest version, I found that these tests were broken because the server appeared to be returning 200 instead of the configured error status.
It looks like this change added a new setting,
AllowAnyHttpStatusCodeInResponse, and that the old behavior— having the server return the exact status that I told it to return— is now only available if I explicitly set that option to true. If I don't, then it silently discards any status code that isn't defined in the System.Net.HttpStatusCode enum, and substitutes 200 instead.This is IMO unexpected and undesirable behavior, for several reasons:
HttpStatusCode is not an exhaustive list of valid HTTP statuses. It's a convenience enum based on the statuses that were initially defined in RFC 2616. HTTP has moved on since then.
The behavior of having it simply pretend that the code I specified was actually 200 does not seem helpful in any scenario I can think of. If you don't want to make it possible to configure the server to return certain statuses, then why not actually indicate that the configuration is invalid, by throwing an exception—or, at the very least, logging a warning? One reason this problem was so hard for me to track down was that I had turned on logging, but the log output indicated that it was going to send a response with the same status that I had configured; that was untrue, it really sent a 200, but that change happened after the response had already been logged.
(Both 1 and 2 are very similar to a problem that I complained about in an earlier issue, regarding the use of less common HTTP methods like REPORT. The HTTP specs do not say that any methods or statuses they don't specifically mention are actually invalid; it's an extensible protocol by design. And even if they were invalid, it is certainly possible that a web server somewhere was written to use them, so it should be possible to test HTTP client code to make sure it would behave properly in such a case. WireMock.Net, as a testing tool, should let you set up any kind of HTTP response that your client might encounter; it should not have opinions about whether such a response could really happen. And silently altering my test conditions is an extremely undesirable behavior for any testing tool; either it should do what I asked it to do, or it should refuse in an easily recognizable way, rather than doing something else instead.)
I know that this project isn't really using semantic versioning, but even so, adding a new feature while also changing the default behavior in a backward-incompatible way is not really appropriate for a micro version update like 1.1.6.
This change in behavior is very poorly documented. There's nothing in the doc comment for
IStatusCodeResponseBuilder.WithStatusCodeto indicate that the method is no longer guaranteed to actually use the status code you specified, and the comment forIWireMockServerSettings.AllowAnyHttpStatusCodeInResponsejust says "Allow any HttpStatusCode in the response", with no mention of what the alternative to that is.I hope this doesn't come across as an overly negative or hostile issue. WireMock.Net is an extremely useful tool and I appreciate the work that's gone into it. But, for that reason, we've come to rely on it pretty heavily and it's worrisome to think that its behavior can change so significantly, and with so little explanation, in a micro version update. I'd feel much better about this if you would consider following the widely used semantic versioning standard.
@StefH commented on GitHub (Feb 15, 2020):
Hello @eli-darkly,
Thank you for your comments. I thought this changed behaviour did not break any backwards compatible logic.
I think it's a better idea to rename the property to
AllowOnlyDefinedHttpStatusCodeInResponsewhich will be default set to false. So this means that all is allowed, except when you set this property to true.I'll fix the code and a new NuGet will be released automatically on MyGet, I'll write the version in this issue.
About semantic versioning : I don't follow this yet for any of my projects. But I need to rethink that and see if I can start on this project.
@StefH commented on GitHub (Feb 15, 2020):
See https://github.com/WireMock-Net/WireMock.Net/pull/422
@StefH commented on GitHub (Feb 17, 2020):
See MyGet -
WireMock.Net.1.2.0-ci-12730.nupkg@StefH commented on GitHub (Feb 23, 2020):
@eli-darkly : Did you have time to check that specific NuGet version which should solve your issue?
@StefH commented on GitHub (Mar 6, 2020):
@eli-darkly : Did you have time to check that specific NuGet version which should solve your issue?
@eli-darkly commented on GitHub (Mar 6, 2020):
It hadn't been a priority because there was a clear workaround (setting the option to true), and because I've never used MyGet and don't actually know how to.
@StefH commented on GitHub (Mar 6, 2020):
No worries. I just want to verify if my fix can be approved by you before I merge the code into master.
You can see this page for information on MyGet https://github.com/WireMock-Net/WireMock.Net/wiki/MyGet-preview-versions
@eli-darkly commented on GitHub (Mar 10, 2020):
@StefH Thanks - I've retested with that prerelease version, and it now works without me having to set AllowAnyHttpStatusCodeInResponse.
@StefH commented on GitHub (Mar 10, 2020):
@eli-darkly Thank you for testing, I will release a new version, probably 1.2.x.x in the next days.