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

Fixed #29 (Allow partial body matching) #33

Closed
wants to merge 3 commits into from

Conversation

adinapoli
Copy link

Hey @sol , long time no chat!

At work I needed a matcher to be able to match only part of the http response body, and having noticed there was interest in such feature (see #29) I took at crack at it.

My implementation is very simple, so feel free to suggest amendment to it.
It works nicely, also in conjunction with other QuasiQuoters, like Text.RawString.QQ (misc screenshots follow, they do not follow any precise workflow or ordering):

screen shot 2016-03-16 at 14 50 57

screen shot 2016-03-16 at 14 49 05

screen shot 2016-03-16 at 14 43 26

screen shot 2016-03-16 at 14 43 10

What would you like to see in terms of extra stuff (docs, tests, etc) to consider this mergeable?
I have also added support for building this with stack but I hardly think it's a problem!

Thank you!

Alfredo

@adinapoli
Copy link
Author

Argh, noticed the Travis failure. Checking it.

@adinapoli
Copy link
Author

It doesn't seem to be related to my patch:

screen shot 2016-03-17 at 08 58 20

@sol
Copy link
Member

sol commented Apr 3, 2016

@adinapoli Sorry for the delay on this. I think this should be handled as a different matcher instead of a new combinator shouldContainInBody.

The code I just pushed to #34 should allow for this. I'm on the go right now, but I'll add some more feedback when I have time.

@adinapoli
Copy link
Author

No problem @sol , thanks for taking the time to work yourself on this!

I'm struggling to see how I can match only on a portion of the response body with your outstanding patch (and I think that's due to my limited knowledge of the codebase!)

Can you give me an example on how to use it to achieve the behaviour of my PR (see attached screenshots)?

Thank you Simon!

Alfredo

@sol
Copy link
Member

sol commented Apr 7, 2016

@adinapoli we now have a type

data MatchBody = MatchBody ([Header] -> Body -> Maybe String)

With this, you can match arbitrary things on the body, not only equality as it was before. This should also allow for implementing a partial body matcher.

@adinapoli
Copy link
Author

Hey @sol , thanks, I totally missed the new MatchBody type. It looks it might be used for the same purpose of my ad-hoc combinator. I would expect it to work like this:

matchPartial :: ByteString -> MatchBody
matchPartial expected = MatchBody $ \headers (Body b) -> case B.isInfixOf expected b of
  True -> Nothing
  False -> Just "Expected bla bla, got bla bla" 
...
myReq `shouldRespondWith` 200 { matchBody = matchPartial "foobar" }

Am I on the right track? If yes, I think there is no need for the shouldContainInBody combinator, unless you think it's the good syntactic sugar we should give to the user to avoid implementing the 4 line function matchPartial as I did. Your call I guess!

@adinapoli
Copy link
Author

Hey @sol ,

a bit of update from the trenches. I have finally managed to find the time to test out your latest MatchBody addition, and it worked just fine! This is the small combinator I wrote:

matchPartial :: B.ByteString -> MatchBody
matchPartial expected = MatchBody $ \_ body -> case B.isInfixOf expected (toS body) of
  True  -> Nothing
  False -> Just $  "Expected to find somewhere in body: " <> toS expected <> ",\n"
                <> "Found: " <> toS body

And used like so:

      describe "PUT /heracles/organizations/:orgid/sign-up-links/edit/:id" $ do
        it "should edit sign-up-link" $ do
          put "/heracles/organizations/1/sign-up-links/edit/1" (JSON.encode newSul { nsr_usage_limit = Nothing })
            `shouldRespondWith` 200 { matchBody = matchPartial "\"usage_limit\":null" }

Thanks for your support, and I hope this will help the next guy reading this!

Alfredo

@adinapoli adinapoli closed this Jun 20, 2016
@fisx fisx mentioned this pull request Oct 3, 2016
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.

2 participants