Skip to content

Debugging overhaul #456

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

Open
wants to merge 26 commits into
base: main
Choose a base branch
from
Open

Debugging overhaul #456

wants to merge 26 commits into from

Conversation

hadley
Copy link
Member

@hadley hadley commented May 1, 2025

With a focus on logging, which has lead to quite a few minor tweaks to the existing logging output.

@hadley
Copy link
Member Author

hadley commented May 2, 2025

@sckott I think it would be interesting for you to look at the rendered vignette and contemplate if there's a way for me to convince you that logging + a vignette can be better than the custom error messages.

@sckott
Copy link
Collaborator

sckott commented May 2, 2025

@hadley I'll do that. we're packing for a weekend trip then heading off soon, so I won't look at anything until monday :)

@sckott
Copy link
Collaborator

sckott commented May 14, 2025

I like this, thanks.

I guess the allow_playback_repeats situation is still up in the air - not sure why it doesn't work but deciding whether we want it or not comes first

WRT replacing logging with errors.R i'll try that locally and see how it feels

@hadley
Copy link
Member Author

hadley commented May 14, 2025

I put together #488 just to get concrete

@hadley
Copy link
Member Author

hadley commented Jun 2, 2025

@scott @maelle I think this is now ready for you both to take a look and let me know what I'm missing.

@hadley hadley marked this pull request as ready for review June 2, 2025 16:37
@sckott sckott added this to the v2.0 milestone Jun 2, 2025
@hadley
Copy link
Member Author

hadley commented Jun 3, 2025

Thanks @maelle — I was on a plane with crappy wifi.

Copy link
Member

@maelle maelle left a comment

Choose a reason for hiding this comment

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

Thank you, it will be very helpful!

@@ -1,149 +1,205 @@
---
title: "Debugging your tests that use vcr"
Copy link
Member

Choose a reason for hiding this comment

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

the title has to be updated here (the two changes that are in vcr.Rmd at the moment)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ooops

This vignette helps you debug the vcr error that you're most likely to encounter: "Failed to find matching request in active cassette.".
If you're lucky, the request has genuinely changed, and you can make this problem go away by deleting the previous cassette so vcr can re-record it 🙂.
Otherwise, you'll need to do some debugging.
First we'll talk about logging, and how you can use it to better understand vcr's request matching process.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
First we'll talk about logging, and how you can use it to better understand vcr's request matching process.
First we'll talk about logging, and how you can use it to better understand vcr's request matching process (comparing your real request against any requests stored in cassettes).

maybe put something in parentheses like above?

@hadley
Copy link
Member Author

hadley commented Jun 9, 2025

I think I got all the changes, please let me know if I missed anything!

@maelle
Copy link
Member

maelle commented Jun 10, 2025

Thank you! To me the only thing missing is a brief explanation of what local() is, to remove potential confusion.

@sckott
Copy link
Collaborator

sckott commented Jun 10, 2025

All LGTM other than @maelle question about local

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.

3 participants