Skip to content
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.

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

Handle Pinpoint error without "message" #4507

Merged
merged 8 commits into from Dec 6, 2019

Conversation

@ericclemmons
Copy link
Member

ericclemmons commented Dec 3, 2019

Issue #, if available: #4485

@mariotsi discovered in #4485:

From our remote logs we saw errors coming from this line TypeError: Cannot read property 'startsWith' of undefined. We are unable to check more deeply what was the response because we do not not log payloads. I'm making this change in order to make the code more failsafe

This PR fixes a unit test that was incorrectly asserting that a rejection happened, not what got rejected, and adds a test case for the behavior @mariotsi is seeing.

The primary difference between aa8e3de and this PR is that message is cast as a String to defensively catch undefined, null, booleans, numbers, or any other valid, non-String JSON responses. It also avoids defaulting message to "", so that the original value of undefined` is preserved for future changes.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

mariotsi and others added 7 commits Nov 29, 2019
From our remote logs we saw errors coming from this line `TypeError: Cannot read property 'startsWith' of undefined`. We are unable to check more deeply what was the response because we do not not log payloads. I'm making this change in order to make the code more failsafe
analytics.configure(options);
const spyon = jest
.spyOn(Pinpoint.prototype, 'updateEndpoint')
.mockImplementationOnce((params, callback) => {
callback({ message: 'error' }, null);

This comment has been minimized.

Copy link
@ericclemmons

ericclemmons Dec 3, 2019

Author Member

This test was rejecting with callback is not a function and still passing, since it wasn't mocking this behavior correctly:
https://github.com/aws-amplify/amplify-js/blob/master/packages/analytics/src/Providers/AWSPinpointProvider.ts#L405-L407


test('BAD_REQUEST_CODE without message rejects error', async () => {
const analytics = new AnalyticsProvider();
const mockError = { debug: 'error', statusCode: 400 };

This comment has been minimized.

Copy link
@ericclemmons

ericclemmons Dec 3, 2019

Author Member

Prior to the fix in a1b13da, this fails.

@ericclemmons ericclemmons marked this pull request as ready for review Dec 3, 2019
@ericclemmons ericclemmons requested a review from iartemiev Dec 3, 2019
@ericclemmons ericclemmons self-assigned this Dec 3, 2019
@codecov

This comment has been minimized.

Copy link

codecov bot commented Dec 3, 2019

Codecov Report

Merging #4507 into master will increase coverage by 0.06%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4507      +/-   ##
==========================================
+ Coverage   76.18%   76.24%   +0.06%     
==========================================
  Files         174      174              
  Lines        9619     9619              
  Branches     1964     1906      -58     
==========================================
+ Hits         7328     7334       +6     
- Misses       2146     2148       +2     
+ Partials      145      137       -8
Impacted Files Coverage Δ
...ges/analytics/src/Providers/AWSPinpointProvider.ts 71.55% <100%> (+1.83%) ⬆️
packages/core/src/OAuthHelper/GoogleOAuth.ts 31.25% <0%> (ø) ⬆️
...ackages/pubsub/src/Providers/AWSAppSyncProvider.ts 66.66% <0%> (ø) ⬆️
...pubsub/src/Providers/AWSAppSyncRealTimeProvider.ts 18.64% <0%> (ø) ⬆️
...rc/Providers/AmazonAIConvertPredictionsProvider.ts 61.23% <0%> (ø) ⬆️
packages/xr/src/Providers/SumerianProvider.ts 47.55% <0%> (ø) ⬆️
packages/core/src/OAuthHelper/FacebookOAuth.ts 34.09% <0%> (ø) ⬆️
packages/auth/src/OAuth/OAuth.ts 48.48% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8d2ba6e...bfd9219. Read the comment docs.

@mariotsi

This comment has been minimized.

Copy link

mariotsi commented Dec 4, 2019

LGTM. I agree that preserving undefined is more future proof.
Thanks for you time investigating this.

Copy link
Contributor

iartemiev left a comment

LGTM

@ericclemmons ericclemmons merged commit a63261d into aws-amplify:master Dec 6, 2019
5 checks passed
5 checks passed
LGTM analysis: Python No code changes detected
Details
LGTM analysis: JavaScript No new or fixed alerts
Details
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: unit_test Your tests passed on CircleCI!
Details
codecov/changes No unexpected coverage changes found.
Details
@ericclemmons ericclemmons deleted the ericclemmons:mariotsi-patch-1 branch Dec 6, 2019
elorzafe added a commit that referenced this pull request Dec 18, 2019
* Handle undefined `message`

From our remote logs we saw errors coming from this line `TypeError: Cannot read property 'startsWith' of undefined`. We are unable to check more deeply what was the response because we do not not log payloads. I'm making this change in order to make the code more failsafe

* Test was was incorrectly calling "callback" instead of asserting rejection

* Prettier formatting

* Add unit test for BAD_REQUEST_CODE

* Cast message as String vs. defaulting value
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.