-
Notifications
You must be signed in to change notification settings - Fork 12
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
failure on Windows #158
Comments
did you try setting |
Thanks! I get new failures now https://github.com/ropensci/ropenaq/runs/470099508 |
it looks like you have the same cassettes though - i should have said use |
I thought I had ropensci-archive/ropenaq@3be1989 but I'll retry |
This is my config library("vcr")
invisible(vcr::vcr_configure(
dir = "../fixtures",
preserve_exact_body_bytes = TRUE
)) Re-recording doesn't seem to change much ropensci-archive/ropenaq@d68e840 |
what version of |
0.4.0, I'll upgrade to the dev version! |
Ok, I updated webmockr and vcr using their dev version (and set the https://github.com/ropensci/ropenaq/runs/471886535?check_suite_focus=true An HTTP request has been made that vcr does not know how to handle:
GET https://api.openaq.org/v1/measurements?country=DE&city=J%C3%AF%C2%BF%C2%BDrgen%20Friesel&limit=1&page=1
vcr is currently using the following cassette:
- ../fixtures/buildQueries_accents.yml
- record_mode: once
- match_requests_on: method, uri It's a test called "Queries work with spaces and accents". Cassette; test. |
thanks, I'll have a look |
works fine locally. ugh, i guess an encoding issue on windows only. imagine it's in the code for comparing requests. |
I was able to replicate the issue on a windows VM - I can't figure out what the non-escaped query string should look like for the failing test. I get curl::curl_unescape("J%EF%BF%BDrgen+Friesel")
#> [1] "J�rgen+Friesel" to clarify (and still on windows): I could only replicate the error when using your original casette file in your github repo. Hoewever, if I deleted the cassette file, then ran tests again, it did not fail. So perhaps the cassette is recorded on a different operating system than where the error is happening, and somehow the encoding is handled differently |
So how should I proceed to try and help debug this next week? |
we do a pretty simple comparison of the URIs here https://github.com/ropensci/vcr/blob/master/R/request_matcher_registry.R#L48 - i guess we need to figure out a way to make sure the two strings are not messed up on windows, not sure yet |
maybe the two URIs should be compared after being |
New platform to reproduce the error 😏 https://cloud.r-project.org/web/checks/check_results_ropenaq.html i.e. https://blog.r-hub.io/2019/04/25/r-devel-linux-x86-64-debian-clang/#r-devel-linux-x86-64-debian-clang |
thanks, okay, but we can't test |
Right. That said any R-hub Windows platform should reproduce the bug. |
Sorry for the delay on this @maelle - I assume this is still a problem, yes? If so, what branch should I use to test |
I think so but haven't looked into it for a long time (thankfully you know I'll soon spend more time thinking about http testing!) I see another test is failing at the moment, unrelated to vcr, sorry about that. |
This comment has been minimized.
This comment has been minimized.
I tried adding It'd be very nice to fix this. 🤔 |
Could it work to encode the uri at the time of serializing it to YAML? |
With JSON the failure is different. https://github.com/maelle/vcrencoding/runs/1464200742?check_suite_focus=true |
Did this get sorted yet? |
I don't think so, do you need write access to https://github.com/maelle/vcrencoding? |
Hi @sckott & @maelle, |
Thanks for chiming in here @yogat3ch A reprex would be great |
@sckott Ok, finally figured out how to make a reprex for this behavior. It's a tricky one:
It definitely has to do with the URL encoding because if Thank you for looking into this! |
thanks @yogat3ch will have another look at this |
cf https://github.com/ropensci/ropenaq/runs/469919162?check_suite_focus=true
I'm guessing it's an encoding issue 😉 😭
The text was updated successfully, but these errors were encountered: