[Refactor] Improve Retryer specs #86
Conversation
| if !retried | ||
| retried = true | ||
| raise error_class.new("nope") | ||
| described_class::DO_NOT_RETRY_ERRORS.each do |error_class| |
There was a problem hiding this comment.
I'd advise against this change because it assumes described_class::DO_NOT_RETRY_ERRORS is specified correctly and digs into implementation details. I'd rather the test be "adversarial" and assert what correct behavior should be.
Also, AFAICT, it no longer tests the cases where we do want to retry. It seems wrong not to test the other side.
There was a problem hiding this comment.
@drewhoskins-stripe do you have a failure mode in mind? Would it be someone adding an error to the list that should be retryable instead? But there isn't a great way to test against that unless you run test all possible error classes, which is not a real solution.
The retry case is covered by the spec above this one, but I see now that it's a bit different, I'll add the missing bit.
There was a problem hiding this comment.
Actually looking at this again… the retry case is identical to the spec above because of the if !retried escape hatch — in the case when a retry is expected the error wasn't getting raised outside of the .with_retries block, so therefore not picked up by rescue and not checked. This is however covered by a few specs above, e.g. "backs off and stops retrying eventually"
There was a problem hiding this comment.
These are not the same test. In one case, you retry a few times. In this one, you don't retry at all.
There was a problem hiding this comment.
do you have a failure mode in mind?
Yep, it's a bit subtle but worth discussing.
Tests are the documentation of what the code is expected to do. They represent the "contract."
If a test of the public-facing API changes, a code reviewer should scrutinize it more.
Suppose the retryer is used in several locations. Perhaps an author removes one of these errors because, for their place where the retryer is being used, we should retry.
A test should fail to alert the author (and code reviewer) that a key contract is being changed, and can go look at where the retryer is used. They can then have the conversation of whether the retryer should have a dynamic list of errors that's passed in.
There was a problem hiding this comment.
These are not the same test. In one case, you retry a few times. In this one, you don't retry at all.
I don't get it. In the current version you would retry once and then throw, which is no different from say "can succeed after retries" test case or "executes the on_retry callback" if the intention is to count callback calls. What exact use-case do you think is not covered after the proposed change?
I agree that having a "contract" in place is beneficial, it practise what would happen is whoever removes an error from the list will also update the spec with an equal change. But there's another side to it — someone might add an error to the list and not update the spec. In this case the spec will not fail and the contract gets broken. That can probably be solved by a specific test that checks which exact errors are listed as non-retriable
| GRPC::Unimplemented, | ||
| ] | ||
| end | ||
| DO_NOT_RETRY_ERRORS = [ |
…ginated-workflows added paginated list_workflows_executions
A few improvements:
No functional changes