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

Improve testing guide and add HTTP adapter via Application config #9

Closed
wants to merge 1 commit into from

Conversation

codeadict
Copy link
Collaborator

Adds a new behavior-based adapter that makes it easier to test HTTP clients with Mox, inspired by how Tesla does it. It also improves the testing documentation based on this new feature.

This will allow us to stop the pattern of creating a higher-level behaviour that requires writing more code and we are mocking the actual implementation instead of the low-level HTTP/TCP calls. See the thread for more details. This will also help with reducing the amount of code we have for a single HTTP client as a side effect since DevAdapters can also be using this behaviour and return faked data by pattern matching in the URL and/or it's parameters as seen in https://github.com/codeadict/testing_carreq/blob/main/lib/colour_lovers/dev_adapter.ex#L6.

@codeadict codeadict added documentation Improvements or additions to documentation enhancement New feature or request labels Dec 13, 2023
@codeadict codeadict self-assigned this Dec 13, 2023
Copy link

@iacutone iacutone left a comment

Choose a reason for hiding this comment

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

I really like this pattern–it allows us to lean into Mox when needed or "stub" out an http response, neat!

Additionally, it takes advantage of the config injection pattern already established on cars platform.

Copy link

@jersearls jersearls left a comment

Choose a reason for hiding this comment

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

This is awesome. Let's do it!

@stelane
Copy link
Collaborator

stelane commented Dec 26, 2023

this looks great!
do we need to consider this being a breaking change for testing when we up the version? Is it backwards compatible with our current testing scheme?

@codeadict
Copy link
Collaborator Author

this looks great! do we need to consider this being a breaking change for testing when we up the version? Is it backwards compatible with our current testing scheme?

@stelane This is backwards compatible, we have different ways of testing across the app now. Mainly:

  1. Creating a behaviour and mocking that behaviour entirely at a high level instead of the HTTP layer which doesn't mock the HTTP calls and omits testing any private function that does something with the responses.
  2. Using bypass which ends up having issues in CI like making real requests sometimes due to bypass ports not being passed correctly, or flaky tests due to bypass timeouts.
  3. Adding an additional opts param to HTTP client functions and in tests pass the adapter to it. This is a good approach with the downside you still write a lot of code to create a behaviour and then a dev adapter that implements this behaviour.

The approach proposed here is similar but offers the ability to reduce the amount of code and offers a documented way we we progressively move all the HTTP clients to.

@wojtekmach
Copy link

Hey everyone! I just stumbled upon this issue while searching for some previous conversations and yeah, I've been thinking along similar lines for quite some time. I created a proposal for API changes in Req. I also have an early article draft with some more context too. Any feedback welcome!

@ethangunderson
Copy link
Collaborator

I just noticed that this is still open. Is there anything blocking us from moving forward with this implementation?

@codeadict
Copy link
Collaborator Author

@ethangunderson - Given the comments from @wojtekmach we probably should close this in favor of the native implementation in req that is somewhat similar. I didn't want to introduce something temporary that we will rewrite later on. What do you think?

@ethangunderson
Copy link
Collaborator

@codeadict I think that's fair.

@wojtekmach What kind of assistance can we give you for the Req implementation?

@wojtekmach
Copy link

@ethangunderson just let me know if wojtekmach/req#287 sounds reasonable enough. :) I implemented parts of the proposal on main already.

@wojtekmach
Copy link

Hey folks, just a heads up that I released Req v0.5 with a focus on testing! We added a way to simulate network errors and set request expectations (e.g. multiple expectations that an endpoint is hit, in order). More details here: https://dashbit.co/blog/req-v0.5. As always, if I can help in any way, let me know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants