Skip to content
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

Both assertHeaderExists functions now behave in the same way. #323

Merged
merged 1 commit into from
Sep 20, 2016
Merged

Both assertHeaderExists functions now behave in the same way. #323

merged 1 commit into from
Sep 20, 2016

Conversation

rbudzko
Copy link
Contributor

@rbudzko rbudzko commented Sep 16, 2016

While not sure if it is a desired behavior, I'd like to propose such a change 😇 .

@akka-ci
Copy link

akka-ci commented Sep 16, 2016

Can one of the repo owners verify this patch?

@rbudzko
Copy link
Contributor Author

rbudzko commented Sep 17, 2016

BTW, it's #5.

@ktoso
Copy link
Contributor

ktoso commented Sep 17, 2016

This does not solve #5 though? I was about making the error message show what headers were available in addition to saying what was missing.

@ktoso
Copy link
Contributor

ktoso commented Sep 17, 2016

OK TO TEST

@akka-ci akka-ci added validating PR that is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed validating PR that is currently being validated by Jenkins labels Sep 17, 2016
@akka-ci
Copy link

akka-ci commented Sep 17, 2016

Test PASSed.

@rbudzko
Copy link
Contributor Author

rbudzko commented Sep 17, 2016

This does not solve #5 though? I was about making the error message show what headers were available in addition to saying what was missing.

Another assertHeaderExists (called inside) is stating that header was missing or that it was present but none of given values was compliant with the assertion: header was found but had the wrong value. Found headers: ${headers.mkString(", ")}").

@ktoso
Copy link
Contributor

ktoso commented Sep 17, 2016

Please mention such things in PRs ;) It's non obvious to see from the changes made that now it's supposed to work as intended

@ktoso
Copy link
Contributor

ktoso commented Sep 17, 2016

LGTM

@rbudzko
Copy link
Contributor Author

rbudzko commented Sep 17, 2016

Please mention such things in PRs ;) It's non obvious to see from the changes made that now it's supposed to work as intended

Uh, sorry. Will do in future.

@jypma
Copy link
Contributor

jypma commented Sep 19, 2016

Thanks for the PR! Even more important than the PR text is the commit message. If it resolves #5, mention it in the message. And be specific: rather than saying "in the same way", explain what that way is.

That way, anyone browsing the code at a future point in time can immediately see the intentions, without having to do further digging.

@akka-ci akka-ci added validating PR that is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed tested PR that was successfully built and tested by Jenkins validating PR that is currently being validated by Jenkins labels Sep 19, 2016
@akka-ci
Copy link

akka-ci commented Sep 19, 2016

Test PASSed.

@akka-ci akka-ci added validating PR that is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed tested PR that was successfully built and tested by Jenkins validating PR that is currently being validated by Jenkins labels Sep 19, 2016
@akka-ci
Copy link

akka-ci commented Sep 19, 2016

Test PASSed.

@rbudzko
Copy link
Contributor Author

rbudzko commented Sep 19, 2016

Thanks for the feedback! I've improved the commit message.

@jypma
Copy link
Contributor

jypma commented Sep 19, 2016

Commit message now has a typo (too many 's' in 'missing') :)

And "misssing header name or values not matching assertion are not logged"? Isn't the whole point that they now are logged?

@rbudzko
Copy link
Contributor Author

rbudzko commented Sep 19, 2016

Pardon my absentmindedness...

@jrudolph
Copy link
Contributor

Thanks @rbudzko. Great change.

@akka-ci akka-ci added validating PR that is currently being validated by Jenkins and removed tested PR that was successfully built and tested by Jenkins labels Sep 19, 2016
@akka-ci akka-ci added needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed validating PR that is currently being validated by Jenkins labels Sep 19, 2016
@akka-ci
Copy link

akka-ci commented Sep 19, 2016

Test FAILed.

@jypma
Copy link
Contributor

jypma commented Sep 19, 2016

[info]   - the config setting (chunked entity) *** FAILED ***
[info]     null did not equal EntityStreamSizeException: actual entity size (None) exceeded content length limit (10 bytes)! You can configure this by setting `akka.http.[server|client].parsing.max-content-length` or calling `HttpEntity.withSizeLimit` before materializing the dataBytes stream.,
[info]     inside HttpResponse(200 OK,List(),HttpEntity.Chunked(application/octet-stream),HttpProtocol(HTTP/1.1)) (LowLevelOutgoingConnectionSpec.scala:564)

is that even related to the change?

@@ -207,8 +199,8 @@ abstract class TestRouteResult(_result: RouteResult, awaitAtMost: FiniteDuration
* Assert that a header of the given name and value exists.
*/
def assertHeaderExists(name: String, value: String): TestRouteResult = {
val lowercased = name.toRootLowerCase
Copy link
Contributor

Choose a reason for hiding this comment

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

This was intentional. Otherwise, the function given to the filter will call name.toRootLowerCase for every header during the iteration.

/**
* Assert that a given header instance exists in the response.
*/
def assertHeaderExists(expected: HttpHeader): TestRouteResult = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you remove the braces? Thanks ;)

…ation about missing header name or values not matching assertion are logged. Addresses (#5).
@akka-ci akka-ci added validating PR that is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) validating PR that is currently being validated by Jenkins labels Sep 20, 2016
@akka-ci
Copy link

akka-ci commented Sep 20, 2016

Test PASSed.

Copy link
Contributor

@aruediger aruediger left a comment

Choose a reason for hiding this comment

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

Looks good @rbudzko! I'd kepp the first line short:

assertHeaderExists functions now behave in the same way  (#5)
information about missing header name or values not matching assertion are logged.

But we can fix that while merging.

@akka/akka-http-team Do we have a policy re. commit msgs yet?

@jrudolph
Copy link
Contributor

@akka/akka-http-team Do we have a policy re. commit msgs yet?

I'd say we try to keep the akka one for consistency.

@aruediger aruediger merged commit 73d4f72 into akka:master Sep 20, 2016
@aruediger
Copy link
Contributor

Thx @rbudzko!

@rbudzko rbudzko deleted the assert-header-exists branch September 20, 2016 13:21
@aruediger
Copy link
Contributor

I'd say we try to keep the akka one for consistency.

see akka/akka-meta#30

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tested PR that was successfully built and tested by Jenkins
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants