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

Make debugging unmatched requests easier #14

Merged
merged 2 commits into from
Mar 5, 2024

Conversation

amosjyng
Copy link
Contributor

Hey, thanks for the great library! I added some print statements in my fork because I found it cumbersome to debug failing tests. I can hide this logic behind a flag or something if you're interested in merging this PR

Copy link
Contributor

@mksh mksh left a comment

Choose a reason for hiding this comment

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

Hello! Thank you for the contribution. This idea looks good overall, but PR needs some changes to get it through

  • please make diff printing functionality to be optional, with VCRMiddleware method named like with_rich_diff turning it on
  • please use tracing::info! macro to print diff instead of eprintln!(), so output can be managed by RUST_LOG environment configs
  • please add integration test that has unmatched request sent, and verifies the diff is printed in correct form (can use tracing-test https://github.com/dbrgn/tracing-test library to capture what is being printed in tests).

Thanks in advance!

@amosjyng
Copy link
Contributor Author

Thanks for the review! I'm currently trying to make a deadline, but I'll give this a pass in a few weeks if that's ok :)

@mksh
Copy link
Contributor

mksh commented Jan 25, 2024

Yeah that's fine for sure, follow up on your discretion I am always open for review!

@amosjyng
Copy link
Contributor Author

Ok, I've made an initial pass at implementing the suggestions! Some notes:

  • I changed the implementation of Middleware.handle to return a reqwest_middleware::Error::Middleware instead of panicking, because it seems hard to test for emitted logs after panicking. In particular, catch-unwind does not take async functions. This would also allow for usage of this library in non-test contexts.
  • Due to the above, I added anyhow as a crate dependency because request-middleware v0.1.6 depends on anyhow already, and reqwest_middleware::Error expects an anyhow::Error
  • While tracing_test::traced_test successfully prints out the entire debug output, it appears to only save the first line when it comes time to call logs_contain. As such, I split each line of output when emitting the trace, and recombine them at the end of the test.

Let me know what you think :)

Copy link
Contributor

@mksh mksh left a comment

Choose a reason for hiding this comment

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

LGTM

@mksh mksh merged commit 784db39 into ChorusOne:main Mar 5, 2024
1 check passed
@mksh
Copy link
Contributor

mksh commented Mar 5, 2024

Released 0.2.1 version with changes: https://crates.io/crates/rvcr/0.2.1

@amosjyng
Copy link
Contributor Author

amosjyng commented Mar 5, 2024

Nice, thank you!

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.

None yet

2 participants