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

Parametric mock federator #1558

Merged
merged 3 commits into from
Jun 2, 2021
Merged

Conversation

pcapriotti
Copy link
Contributor

@pcapriotti pcapriotti commented Jun 1, 2021

This makes the fake response provided to a mock federator take a request argument. The motivation is that it allows a single mock to be used to provide multiple fake responses depending on the requests received.

For example, when testing the endpoint for adding remote members to conversations, the mock federator will be queried twice: once to fetch remote user profiles, and once to propagate the join event to the remote backend. Therefore, we need to be able to set up different fake responses for the two requests.

See this commit in #1556 for a usage example.

@pcapriotti pcapriotti marked this pull request as ready for review June 1, 2021 09:30
Copy link
Member

@jschaul jschaul left a comment

Choose a reason for hiding this comment

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

Could you explain what this PR does and how a mock function with a Request -> a argument is more useful than one with an a argument? It appears whatever the new functionality is, it's not made use of anywhere, so I'm a little bit lost here.

@@ -1671,19 +1671,19 @@ withTempMockFederator ::
(MonadIO m, ToJSON a) =>
Opts.Opts ->
Domain ->
a ->
(FederatedRequest -> a) ->
Copy link
Member

Choose a reason for hiding this comment

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

I think I don't quite understand what this does and how the resp / Request -> a argument should be used. Could you add some inline comments here to these arguments?

@jschaul
Copy link
Member

jschaul commented Jun 1, 2021

(also, this PR needs a merge/rebase due to conflicts)

@pcapriotti pcapriotti force-pushed the pcapriotti/parametric-mock-federator branch from 4014e47 to c150824 Compare June 2, 2021 05:08
@pcapriotti
Copy link
Contributor Author

Could you explain what this PR does and how a mock function with a Request -> a argument is more useful than one with an a argument? It appears whatever the new functionality is, it's not made use of anywhere, so I'm a little bit lost here.

Sorry for not writing any PR description. I've added one now, hopefully that clarifies the reason for these changes. There might be some other way to test endpoints involving multiple federated calls, but making the fake response argument depend on the request seems the most straightforward.

@pcapriotti pcapriotti merged commit 959fc08 into develop Jun 2, 2021
@pcapriotti pcapriotti deleted the pcapriotti/parametric-mock-federator branch June 2, 2021 10:27
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