Skip to content

Conversation

@markekraus
Copy link
Contributor

@markekraus markekraus commented Oct 8, 2017

  • Adds /ResponseHeaders/ Test to WebListener
  • Adds -Query parameter to Get-WebListenerUrl which accepts a dictionary where keys are wuery string fields and values are query string values
  • Replaces http://httpbin.org/response-headers tests with WebListener

Reference #2504

(note: the tests as they were intended before were actually not accurate. http://httpbin.org/response-headers returns an Content-Type: application/json in addition to Content-Type: from the supplied header. The implementation in this PR does allow for a blank/missing Content-Type to be returned. It is possible this may reveal errors that were not visible before.)

Copy link
Collaborator

@iSazonov iSazonov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM with two minor comments.

#region test/tools/WebListener/README.md Overrides
- test/tools/WebListener/README.md
ResponseHeaders
#endregion
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are they really all used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure what you are asking. But, I added a file level override because I had been putting all of the overrides for this in the Global dictionary. That was probably a poor decision, so I'm adding the new section and will do a separate PR after this is merged to move the ones from my previous PRs from global to the file override.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. Clear.

Closed.

}
return JsonConvert.SerializeObject(headers);
}
public IActionResult Error()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add newline.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed.

@markekraus
Copy link
Contributor Author

Travis CI fail due to #5062

@iSazonov
Copy link
Collaborator

iSazonov commented Oct 9, 2017

@markekraus MSFT maintainers address such issues very fast. Be patient :-)

@markekraus
Copy link
Contributor Author

markekraus commented Oct 9, 2017

@iSazonov they do 😄. But the community can also address them if an issue is filed. Besides, I prefer to have an issue that explains why my PRs are failing externally. Makes it easier to track when the external issue has been resolved to move forward. @SteveL-MSFT would likely create an issue for this when he came online anyway. I'm no no hurry, just trying to be helpful and thorough.

@SteveL-MSFT
Copy link
Member

@markekraus looking at the Mac issue, restarted your build for now

@markekraus
Copy link
Contributor Author

@TravisEz13 Is there something outstanding on this PR I need to do?

@TravisEz13 TravisEz13 merged commit b4e8e9d into PowerShell:master Oct 18, 2017
@markekraus markekraus deleted the ReplaceHttpBinResponseHeaders branch January 19, 2018 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants