Skip to content

LG-13191 rescue faraday servererror #10533

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.

Sign up for GitHub

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

Merged
svalexander merged 7 commits into from
May 2, 2024
Merged

LG-13191 rescue faraday servererror #10533

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
709a68f
add faraday servererror to network errors
svalexander Apr 30, 2024
d0a7b5b
move error to appropriate job
svalexander Apr 30, 2024
7f0d209
update refresh job spec to include server error
svalexander May 1, 2024
8401797
Use FakeAnalytics instead of stubbing to get clearer specs
zachmargolis May 1, 2024
bc4f62f
log job completed
svalexander May 1, 2024
25f38e3
changelog: Internal, UspsAuthTokenRefreshJob, rescue Faraday::ServerE…
svalexander May 1, 2024
eb8fb68
Merge branch 'main' into shannon/lg-13191-update-network-errors
svalexander May 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/jobs/usps_auth_token_refresh_job.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ def perform
analytics.idv_usps_auth_token_refresh_job_started

usps_proofer.retrieve_token!
rescue Faraday::TimeoutError, Faraday::ConnectionFailed => err
rescue Faraday::TimeoutError, Faraday::ConnectionFailed, Faraday::ServerError => err
analytics.idv_usps_auth_token_refresh_job_network_error(
exception_class: err.class.name,
exception_message: err.message,
Expand Down
27 changes: 8 additions & 19 deletions spec/jobs/usps_auth_token_refresh_job_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,19 +5,13 @@

let(:subject) { described_class.new }
let(:root_url) { 'http://my.root.url' }
let(:analytics) { instance_double(Analytics) }
let(:analytics) { FakeAnalytics.new }
let(:usps_auth_token_cache_key) { UspsInPersonProofing::Proofer::AUTH_TOKEN_CACHE_KEY }

before do
allow(IdentityConfig.store).to receive(:usps_ipp_root_url).and_return(root_url)

allow(Analytics).to receive(:new).
with(
user: an_instance_of(AnonymousUser),
request: nil,
session: {},
sp: nil,
).and_return(analytics)
allow(subject).to receive(:analytics).and_return(analytics)
end

describe 'usps auth token refresh job' do
Expand Down Expand Up @@ -69,24 +63,19 @@
end

context 'auth request throws error' do
it 'still logs analytics' do
it 'catches server errors and logs them as network errors' do
stub_error_request_token

expect(analytics).to receive(
:idv_usps_auth_token_refresh_job_started,
).once
expect(analytics).to receive(
:idv_usps_auth_token_refresh_job_completed,
).once
subject.perform

expect do
subject.perform
end.to raise_error
expect(analytics).to have_logged_event('UspsAuthTokenRefreshJob: Started')
expect(analytics).to have_logged_event('UspsAuthTokenRefreshJob: Network error')
expect(analytics).to have_logged_event('UspsAuthTokenRefreshJob: Completed')
end
end
Copy link
Copy Markdown
Contributor

@zachmargolis zachmargolis May 1, 2024

Choose a reason for hiding this comment

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

I can't select it, but I'd expect the test above to error -- if we're catching a ServerError, this should fail:

        expect do
          subject.perform
        end.to raise_error

because there is no error being thrown

Copy link
Copy Markdown
Contributor Author

@svalexander svalexander May 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

i see what you're saying, it is passing though which is curious. I;m trying to figure out why this is happening.

Copy link
Copy Markdown
Contributor

@zachmargolis zachmargolis May 1, 2024

Choose a reason for hiding this comment

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

If you take off the expect { ... }.to_not raise_error you see that it's throwing an error for a method expectation based on a different logging method called.

so it's technically still throwing an error and the spec technically passes.

If the test was originally written to say expect { ... }.to_not raise_error(Faraday::ServerError) then it would have been a clearer failure here

Copy link
Copy Markdown
Contributor Author

@svalexander svalexander May 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

when i update the test to ...to_not raise_error(Faraday::ServerError) to check what happens it passes

Copy link
Copy Markdown
Contributor

@zachmargolis zachmargolis May 1, 2024

Choose a reason for hiding this comment

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

It passes with to_not raise_error(Faraday::ServerError) because gets this error instead from the stub

       #<InstanceDouble(Analytics) (anonymous)> received unexpected message :idv_usps_auth_token_refresh_job_network_error with ({:exception_class=>"Faraday::ServerError", :exception_message=>"the server responded with status 500"})

I pushed 8401797 to switch out the instance_double for a FakeAnalytics instance which is a better fake logger and has clearer spec failures when the rescue for Faraday::ServerError is removed

Copy link
Copy Markdown
Contributor Author

@svalexander svalexander May 1, 2024

Choose a reason for hiding this comment

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

ahh ok that makes sense! i think we still need the completion log, I'll add it in. Thanks for making this change!


context 'auth request throws network error' do
[Faraday::TimeoutError, Faraday::ConnectionFailed].each do |err_class|
[Faraday::TimeoutError, Faraday::ServerError, Faraday::ConnectionFailed].each do |err_class|
Copy link
Copy Markdown
Contributor Author

@svalexander svalexander May 1, 2024

Choose a reason for hiding this comment

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

@zachmargolis I'm wondering how i can test this to confirm that #login-alarms wouldnt be alerted

Copy link
Copy Markdown
Contributor

@zachmargolis zachmargolis May 1, 2024

Choose a reason for hiding this comment

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

The alarms go off when the jobs have unhandled exceptions, so by handling the exception (and the subject.perform doesn't throw) then the alarms won't go off

it "logs analytics without raising the #{err_class.name}" do
stub_network_error_request_token(
err_class.new('test error'),
Expand Down