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

Fix handling of numeric error codes coming from AWS SDK requests #9538

Merged
merged 3 commits into from Jun 2, 2021

Conversation

medikoo
Copy link
Contributor

@medikoo medikoo commented Jun 1, 2021

Closes: #9511

@codecov
Copy link

codecov bot commented Jun 1, 2021

Codecov Report

Merging #9538 (fd5e40d) into master (1b90dfb) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #9538   +/-   ##
=======================================
  Coverage   86.70%   86.70%           
=======================================
  Files         321      321           
  Lines       12072    12075    +3     
=======================================
+ Hits        10467    10470    +3     
  Misses       1605     1605           
Impacted Files Coverage Δ
lib/aws/request.js 91.08% <100.00%> (+0.27%) ⬆️

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 1b90dfb...fd5e40d. Read the comment docs.

@medikoo medikoo requested a review from pgrzesik June 1, 2021 11:50
pgrzesik
pgrzesik previously approved these changes Jun 1, 2021
Copy link
Contributor

@pgrzesik pgrzesik left a comment

Choose a reason for hiding this comment

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

Great call, just a minor stylistic comment.

@@ -246,7 +271,10 @@ describe('#request', () => {
const awsRequest = proxyquire('../../../../lib/aws/request', {
'aws-sdk': { S3: FakeS3 },
});
return expect(awsRequest({ name: 'S3' }, 'error')).to.be.rejected;
return expect(awsRequest({ name: 'S3' }, 'test')).to.eventually.be.rejected.and.have.property(
Copy link
Contributor

Choose a reason for hiding this comment

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

is return needed here? should it be marked as async and awaited?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically it's needed if we do not await (and effect is same), but I agree that it's better to mark all promise returning functions as async, and to use await when we do not intend to return anything substantial.

I've updated those tests

@medikoo medikoo merged commit 3715989 into master Jun 2, 2021
@medikoo medikoo deleted the 0601-fix-st branch June 2, 2021 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Got error TypeError: name.replace is not a function when try to deploy simple lambda python with localstack
2 participants