Maintenance week #553
Maintenance week #553
Conversation
* The vcrpy version update required the cassette fixtures to be recreated * Update unit tests to account for the new cassette fixtures
There was a problem hiding this comment.
Looks good to me! Nice work on navigating the cassettes.
I remain a little conflicted about them, but think your solution here is an elegant one for the situation at hand (maintenance + python version bump). If we did want to explore non-VCR approaches, perhaps for some routes or things to test, an issue could be started to explore it. But no need to do everything, all at once, during maintenance.
Similar to other ongoing work, I think iterative change and improvements are great, and this feels like the embodiment of that approach.
| w55au72VgtpCFjeDShZXsriSxZUsrmRxJYsrWVzJ4v+TLP5FWTa/nxxnQiAQX/QGuV3MkBr7a0an | ||
| nanTzogbBFBcuGbj1LBdXGuCUjT+JvKJO9q42R/k7NF1zr7ewQ0cNA57dRgBAvltcD2/jP8Ly9YB | ||
| 6pwfAAA= | ||
| string: "" |
There was a problem hiding this comment.
Took me a bit to parse (had to look at original file and new file, not diff, as the diff kind of mangles it), but I think I'm understanding the changes here. My understanding is:
response.body.string: ""was manually setresponse.status.codewas already500so left as-is
If so, cool! Seems like a good use of setting the HTTP status manually.
I was going to propose that maybe it'd be nice to set a manual response like:
<html><p>ERROR! This is a manually created error that simulates OAI-PMH response for 500.</p></html>
But ultimately, maybe that's just as confusing and/or inaccurate. After thinking on it, I think I'm good with just an empty string "" for the body of this simulated 500 HTTP response.
There was a problem hiding this comment.
I did manually set the 500 as well because that handle is now returning a 200 with a record. Agree with your point about creating a fake response string being confusing and inaccurate. It did ultimately feel like too much overhead to support both vcrpy and requests-mock in the test suite but as you say, we can evaluate this more comprehensively later
Purpose and background context
Maintenance week updates
How can a reviewer manually see the effects of these changes?
Run the following command to verify there are no issues running a harvest:
Includes new or updated dependencies?
YES
Changes expectations for external applications?
NO
Developer
Code Reviewer(s)