Skip to content
This repository has been archived by the owner on Apr 22, 2024. It is now read-only.

[WAIT] Issue 14: Add Request to ResponsePredicate #36

Closed
wants to merge 3 commits into from

Conversation

erewok
Copy link
Contributor

@erewok erewok commented Oct 21, 2017

Description

This PR does the following:

  • Adds the Request to ResponsePredicate so that PredicateFailure can pretty print the request as well as the failing response.

  • Removes the Maybe from PredicateFailure (Maybe Request) now that both RequestFailure and ResponseFailure require the request.

  • Adds tests for the following predicates: not500 and unauthorizedContainsWWWAuthenticate

Fixes #14

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 43.284% when pulling 064b660 on issue-14/req_in_resp_preds into 54a05a5 on master.

@coveralls
Copy link

coveralls commented Oct 21, 2017

Coverage Status

Coverage decreased (-0.1%) to 43.284% when pulling d4f6aa5 on issue-14/req_in_resp_preds into d65abc8 on master.

@erewok
Copy link
Contributor Author

erewok commented Oct 22, 2017

@jkarni I think this one's ready, I just need a bit of help thinking through any broader implications (CHANGELOG, etc.).

@@ -429,7 +430,8 @@ finishPredicates p req mgr = go `catch` \(e :: PredicateFailure) -> return $ Jus
where
go = do
resps <- getRequestPredicate (requestPredicates p) req mgr
mapM_ (getResponsePredicate $ responsePredicates p) resps
let responder = getResponsePredicate (responsePredicates p) req
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, this isn't quite right, I think. The actual requests that resulted in the responses resps are not necessarily req. Rather, they are the requests generated by the RequestPredicates based on req.

Copy link
Member

Choose a reason for hiding this comment

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

I have to think a little more about this. It feels like it's becoming evident that the Request/ResponsePredicate distinction is somewhat artificial or misleading. Really what's at stake is something like the distinction between request generator (or generator modifiers) and (request + response) predicates. But separating those two too much is also wrong, since usually one wants to generate certain requests in order to check the responses.

Copy link
Contributor Author

@erewok erewok Oct 23, 2017

Choose a reason for hiding this comment

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

I am glad to have your perspective on this. I was wondering about the difference between ResponsePredicate and RequestPredicate when I was working on this PR, but I figured the distinction is still valid as a RequestPredicate needs a Manager in order to actually issue requests.

I have to confess I didn't look closely enough at finishPredicates, so it's a relief that you caught my mistake, which I can see clearly now.

I agree with what you're saying that checking a particular response is really often about a (Request, Response) pair. I was kind of errantly thinking about bifunctors or profunctors when looking at this code this weekend, but I didn't follow the thought very far.

@erewok erewok changed the title [READY] Issue 14: Add Request to ResponsePredicate [WAIT] Issue 14: Add Request to ResponsePredicate Oct 24, 2017
@erewok
Copy link
Contributor Author

erewok commented Oct 24, 2017

I have been thinking about this PR and I think I'd like to withdraw it. I have some thoughts that I'd like to explore a bit before coming up with a suggestion for representing predicates based on Request + Response versus predicates based only on a Response.

I have been thinking about this in light of this comment from the Predicates module:

-- The idea with all this footwork is to not waste any requests. Rather than
-- generating new requests and only applying one predicate to the response, we
-- apply as many predicates as possible.

While I have some ideas on representing these things, this part is kind of stumping me.

@erewok erewok closed this Oct 24, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants