-
-
Notifications
You must be signed in to change notification settings - Fork 213
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
Allow all headers to be set as Response headers #142
Conversation
Codecov Report
@@ Coverage Diff @@
## master #142 +/- ##
==========================================
- Coverage 67.87% 67.79% -0.09%
==========================================
Files 80 80
Lines 2945 2931 -14
Branches 394 394
==========================================
- Hits 1999 1987 -12
+ Misses 730 727 -3
- Partials 216 217 +1
Continue to review full report at Codecov.
|
@alastairtree can you review this pr? |
I'm not sure I understand the motivation for this change or the original header restriction? Any background? |
_serverForProxyForwarding = FluentMockServer.Start(); | ||
// Assign | ||
var settings = new FluentMockServerSettings { AllowPartialMapping = false }; | ||
_serverForProxyForwarding = FluentMockServer.Start(settings); |
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.
Perhaps names these 2 servers more specifically as it is quite hard to follow. _serverThatReturnsRedirects and serverThatProxies ?
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 understand your comment, however for me it's clear for now. But I will think about this to see if I want to rename the these variables.
_serverForProxyForwarding = FluentMockServer.Start(); | ||
// Assign | ||
var settings = new FluentMockServerSettings { AllowPartialMapping = false }; | ||
_serverForProxyForwarding = FluentMockServer.Start(settings); | ||
_serverForProxyForwarding | ||
.Given(Request.Create().WithPath("/*")) | ||
.RespondWith(Response.Create() | ||
.WithStatusCode(HttpStatusCode.Redirect) | ||
.WithHeader("Location", _serverForProxyForwarding.Urls[0] + "testpath")); |
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.
This creates a server that could redirect indefinitely I think. Request.Create().WithPath("/")
instead?
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.
No, it has to be like this, else test fails.
@@ -364,24 +367,25 @@ public void FluentMockServer_Admin_Mappings_Add_SameGuid() | |||
new object[] { new JsonPathMatcher("$..[?(@.message == 'Hello server')]"), "text/plain" } | |||
}; | |||
|
|||
[Theory] |
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.
Any particular reason for not using the parameterised tests? The test cleanup at the end could also be done by making the class IDisposable if preferred.
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.
For some reason the tests failed on my local machine, so I just fixed it this 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.
Added a couple of minor comments
Thanks a lot! |
Related to:
#137
#136
#126