[supply] add new SUPPLY_UPLOAD_MAX_RETRIES env var to attempt to solve failed Google API calls #21518
Conversation
…e failed Google API calls
supply/spec/client_spec.rb
Outdated
| to_return(status: 403, body: '{"error":{"message":"Ensure project settings are enabled."}}', headers: { 'Content-Type' => 'application/json' }) | ||
|
|
||
| expect(UI).to receive(:error).with("Google Api Error: Invalid request - Ensure project settings are enabled. - Retrying...").exactly(5).times | ||
| expect(UI).to receive(:user_error!).with("Google Api Error: Invalid request - Ensure project settings are enabled.").once |
There was a problem hiding this comment.
One thing that I got bitten by in the past is that when you stub UI.user_error! with expect or allow, that means that the original method won't be called… which means that it won't raise.
As a result, the caller of call_google_api, in your case client.upload_image, won't be interrupted by the Fastlane::Error raised by user_error! in the context of running tests, while it will in the real world.
Depending on the implementation of client.upload_image(…), that could have side effects if that implementation counts on the fact that call_google_api can raise to stop early and not run the rest of the code of that method on error, as that would mean that the fact that we stubbed user_error! would make the method under test behave differently in test environment vs in reality.
I think the easiest way to solve this would be to replace your expect(UI).to receive(:user_error!) with wrapping your call to client.upload_image within expect { … }.to raise_error(Fastlane::Error, "Google Api Error: Invalid request – Ensure project settings are enabled.". That way you'd let user_error! raise and interrupt the code of upload_image mid-air even in the context of testing.
There was a problem hiding this comment.
@AliSoftware That expect on UI.user_error! was already there 🤷 But... would be adding .and_call_original work? 🤔
There was a problem hiding this comment.
Well that would, except that I think rubocop might suggest you to only call .and_call_original on allow(…), not on expect(…)
So you could allow(UI).to receive(:user_error!).with(…).and_call_original… but if you do that and even if you wanted to still keep the expect(UI).to receive(:user_error!).once to assert on it only being called once and not more, you'll still need to wrap your client.upload_image within expect { … }.to raise_error(…) to avoid the test from crashing on the raised error and instead catch (and assert) it.
So at this point since you'll still need the expect { … } .to raise_error, I'm not sure that and_call_original adds much value, instead of not stubbing UI.user_error! at all?
Otherwise if we just `expect(UI).to receive(:user_error!).with(…)` that will mock the `UI.user_error` call in the method being tested... and thus not make it `raise` anymore, which the rest of the implementation of that method will continue executing when run in the context of mocked tests while it would not in the real world, which means we would be testing a different behavior of the method
There was a problem hiding this comment.
@joshdholtz I hope you don't mind, I've pushed a commit to apply the suggested change I was talking about for the spec 🙂
|
@AliSoftware Thank you! I was at the lake yesterday and didn't have my computer on me. Just getting up with the family now so will get this merged and a new release made 💪 |
This retries option was just released see fastlane/fastlane#21518
### Description This updates `fastlane` to workaround the following error while uploading to the Play Store: ``` [08:03:25]: Google Api Error: Unauthorized - Request is missing required authentication credential. Expected OAuth 2 access token, login cookie or other valid authentication credential. See https://developers.google.com/identity/sign-in/web/devconsole-project. ``` See fastlane/fastlane#21518 See related valora-inc/release-automation#57 ### Test plan Triggering a run from the nightly workflow against the custom branch was successful: https://github.com/valora-inc/release-automation/actions/runs/6221137351 However it seems it didn't hit the API error that time and didn't have to retry. We'll keep an eye on this. ### Related issues - Slack [thread](https://valora-app.slack.com/archives/C02D08P412Q/p1695026822446399). ### Backwards compatibility Yes
* Let `supply` upload to the play store * Attempt to update fastlane * Force update of fastlane to latest version * Add workaround for google api issue This retries option was just released see fastlane/fastlane#21518
Motivation and Context
Somewhat fixes #21507
Applying the patch made by @AliSoftware mentioned #21507 (comment)
Description
Added new
SUPPLY_UPLOAD_MAX_RETRIESenv variable to retry failed Google API requests to attempt to "fix" uploads from erroring