Skip to content

Replace HttpBin.org/response-headers Tests with WebListener - #5058

Merged
Travis Plunk (TravisEz13) merged 5 commits into
PowerShell:masterfrom
markekraus:ReplaceHttpBinResponseHeaders
Oct 18, 2017
Merged

Travis Plunk (TravisEz13) merged 5 commits into
PowerShell:masterfrom
markekraus:ReplaceHttpBinResponseHeaders

Conversation

@markekraus

@markekraus Mark Kraus (markekraus) commented Oct 8, 2017

Copy link
Copy Markdown
Contributor
  • 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.)

@iSazonov Ilya (iSazonov) left a comment

Copy link
Copy Markdown
Collaborator

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.

Comment thread .spelling
#region test/tools/WebListener/README.md Overrides
- test/tools/WebListener/README.md
ResponseHeaders
#endregion

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

Travis CI fail due to #5062

@iSazonov

Copy link
Copy Markdown
Collaborator

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

@markekraus

Mark Kraus (markekraus) commented Oct 9, 2017

Copy link
Copy Markdown
Contributor Author

Ilya (@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. Steve Lee (@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
Copy Markdown
Member

Mark Kraus (@markekraus) looking at the Mac issue, restarted your build for now

@markekraus

Copy link
Copy Markdown
Contributor Author

Travis Plunk (@TravisEz13) Is there something outstanding on this PR I need to do?

@TravisEz13
Travis Plunk (TravisEz13) merged commit b4e8e9d into PowerShell:master Oct 18, 2017
@markekraus
Mark Kraus (markekraus) deleted the ReplaceHttpBinResponseHeaders branch January 19, 2018 18:59
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
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