Skip to content

Avoid duplicating ActiveJob's retry machinery#11061

Merged
vrajmohan merged 1 commit intomainfrom
risc-delivery
Aug 12, 2024
Merged

Avoid duplicating ActiveJob's retry machinery#11061
vrajmohan merged 1 commit intomainfrom
risc-delivery

Conversation

@vrajmohan
Copy link
Contributor

changelog: Internal, Error Handling, Avoid duplicating ActiveJob's retry machinery

It may address
https://gitlab.login.gov/lg-people/lg-people-appdev/Melba/backlog-fy24/-/issues/37.

This is an attempt to use ActiveJob's retry machinery properly, so that errors do not bubble up to NewRelic.

Copy link
Contributor

@Sgtpluck Sgtpluck left a comment

Choose a reason for hiding this comment

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

i don't know enough about ActiveJob to say whether this code would be successful, but it does seem like a reasonable approach. i have some suggestions/questions about the tests, though

Copy link
Contributor

Choose a reason for hiding this comment

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

i don't personally really like shared_examples, but since you've set them up, why wouldn't you use them for the different types of errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My preference has been to use shared_examples for larger examples.

Copy link
Contributor

Choose a reason for hiding this comment

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

since they are designed for reuse, it is confusing to me that you would add them and then not use them for all the similar specs -- it makes me think these are different in some way. am i missing something about them that makes them different?

i would prefer to either write out the longer specs explicitly (my actual preference, since i find shared examples hard to read, especially when there are failures), or use them for all examples because of that confusion it creates. does that make sense?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see what you mean. The mocking fixture was different, the call count expectation was different and the risc event payload error message was different. I'll get rid of the shared_examples as they seem to add to the confusion in this case.

Comment on lines 117 to 154
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a way to use webmock to return the right error instead of manually stubbing Faraday?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IIANM, the comments on lines 114-115 were from you and seemed to indicate that it would not simulate the real world behavior. Should I investigate further?

Copy link
Contributor

Choose a reason for hiding this comment

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

LOL I had completely forgotten, you can investigate more if you want, but it's probably fine as is

@vrajmohan vrajmohan marked this pull request as ready for review August 12, 2024 13:52
changelog: Internal, Error Handling, Avoid duplicating ActiveJob's retry machinery

It may address
https://gitlab.login.gov/lg-people/lg-people-appdev/Melba/backlog-fy24/-/issues/37.

This is an attempt to use ActiveJob's retry machinery properly, so that
errors do not bubble up to NewRelic.
@vrajmohan vrajmohan merged commit b8a3933 into main Aug 12, 2024
@vrajmohan vrajmohan deleted the risc-delivery branch August 12, 2024 16:50
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