> Pull requests don't prevent bugs. This is my pet peeve with code reviews. They revolve around (1) only reading the code, not pulling the branch and testing i

> Pull requests don't prevent bugs. This is my pet peeve with code reviews. They revolve around (1) only reading the code, not pulling the branch and testing it, and (2) reading only the changes. One time I was about to give an approval on a PR because the Python code looked fine. Then I thought "eh, maybe I should pull and test this branch, just in case." With the new code in place, there was a marshmallow dataclass ValidationError in another part of the code that was not part of this change. This dataclass was used to ingest a JSON response from an API built by another team, and by default it had strict checking, including rejecting unknown JSON properties. The developer had been testing against an older version of this API that didn't have the new properties, and I was testing with a more recent version. The other team seemed to follow what I think of as the "default JSON API contract" which is that any changes or removal of existing properties need to be coordinated with the consumers of the API, but they felt free to add new properties to the JSON object. And this broke the strict marshmallow validation. The dataclass had been added some time ago, but this problem never arose because we weren't actually using this API until the new code in the PR started calling it. The fix was simple enough; I just added this inside the dataclass: class Meta: """Ignore unknown fields that we don't use.""" unknown = marshmallow.EXCLUDE You might ask, "Shouldn't this problem have been caught by a unit test?" But realistically, who is ever going to write a unit test that says "let's add some random new property to the mock JSON response and make sure it doesn't raise an exception"? Especially when there are some 50 API calls like this. In the case of a JSON API response created by some loosely-coupled team, you're better off not wasting the time on that. Instead, keep the strict checks and unit tests on properties you rely on, and use the marshmallow.EXCLUDE to ignore new ones that you don't care about. Whether you agree with that or not, this is the kind of bug that no one would ever catch in a "read the code changes" review. It reminds me of the quote attributed to Donald Knuth: "Beware of bugs in the above code; I have only proved it correct, not tried it."