refactor: Mark functions async in `aws/customResources` and `aws/deploy` #8698

ifitzsimmons · 2021-01-01T19:16:08Z

Refactored Promise-returning functions for:

lib/plugin/aws/customResources
lib/plugin/aws/deploy

I had to refactor a few tests after tagging functions with async keyword. I didn't change the functionality of the test, just the syntax.

Addresses: #8368

codecov · 2021-01-01T19:18:38Z

Codecov Report

Merging #8698 (9a7565e) into master (0384776) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #8698      +/-   ##
==========================================
- Coverage   87.76%   87.74%   -0.02%     
==========================================
  Files         264      264              
  Lines        9780     9779       -1     
==========================================
- Hits         8583     8581       -2     
- Misses       1197     1198       +1

Impacted Files	Coverage Δ
...stomResources/resources/cognitoUserPool/handler.js	`0.00% <ø> (ø)`
...urces/resources/cognitoUserPool/lib/permissions.js	`0.00% <ø> (ø)`
...esources/resources/cognitoUserPool/lib/userPool.js	`0.00% <ø> (ø)`
...s/customResources/resources/eventBridge/handler.js	`0.00% <ø> (ø)`
...Resources/resources/eventBridge/lib/eventBridge.js	`0.00% <ø> (ø)`
...Resources/resources/eventBridge/lib/permissions.js	`0.00% <ø> (ø)`
...lugins/aws/customResources/resources/s3/handler.js	`0.00% <ø> (ø)`
...ins/aws/customResources/resources/s3/lib/bucket.js	`0.00% <ø> (ø)`
...ws/customResources/resources/s3/lib/permissions.js	`0.00% <ø> (ø)`
lib/plugins/aws/customResources/resources/utils.js	`24.07% <ø> (ø)`
... and 11 more

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 0384776...9a7565e. Read the comment docs.

pgrzesik

Thank you @ifitzsimmons 🙇 Overall it looks good, I've had a few minor comments, but nothing major 👍

pgrzesik · 2021-01-04T09:39:28Z

lib/plugins/aws/lib/naming.js

@@ -53,7 +53,7 @@ module.exports = {
      this.provider.serverless.service.provider.stackName &&
      typeof this.provider.serverless.service.provider.stackName === 'string'
    ) {
-      return `${this.provider.serverless.service.provider.stackName}`;
+      return this.provider.serverless.service.provider.stackName;


good catch 👍

pgrzesik · 2021-01-04T09:41:59Z

test/unit/lib/plugins/aws/deploy/lib/extendedValidate.test.js

-        expect(fileExistsSyncStub.calledOnce).to.equal(true);
-        expect(readFileSyncStub.calledOnce).to.equal(true);
-      });
+      await awsDeploy.extendedValidate();


Does it have to be await-ed here? According to the changes to extendedValidate, that function is no longer async

Ah, good catch. I'll fix all of those await's.

pgrzesik · 2021-01-04T09:42:10Z

test/unit/lib/plugins/aws/deploy/lib/extendedValidate.test.js

-        expect(fileExistsSyncStub.calledOnce).to.equal(true);
-        expect(readFileSyncStub.calledOnce).to.equal(true);
-      });
+      await awsDeploy.extendedValidate();


Same as above

pgrzesik · 2021-01-04T09:42:39Z

test/unit/lib/plugins/aws/deploy/lib/extendedValidate.test.js

-        delete awsDeploy.serverless.service.package.artifact;
-      });
+
+      await awsDeploy.extendedValidate();


pgrzesik · 2021-01-04T09:42:44Z

test/unit/lib/plugins/aws/deploy/lib/extendedValidate.test.js

-        ].join('');
-        expect(awsDeploy.serverless.cli.log.firstCall.calledWithExactly(msg)).to.be.equal(true);
-      });
+      await awsDeploy.extendedValidate();


ifitzsimmons · 2021-01-05T02:01:06Z

@pgrzesik, I'm having a little trouble merging master back in to my branch. It looks like once I merge it, there are some
strange errors that don't have much to do with my branch. At the very last, there weren't any manual merges required for any of those files.

pgrzesik · 2021-01-05T08:51:10Z

Hey @ifitzsimmons, sorry for the trouble. Yesterday we had some upgrades to internal dependencies which are most likely causing issues for you - reinstalling all deps locally after you merge master back or rebase your branch on top of it should do the trick. Also, we switched to v2 of prettier, so there might be some minor conflicts for you to resolve.

ifitzsimmons · 2021-01-05T15:12:33Z

Okay cool, I wasn't sure how you felt about a rebase during the review process but I'll do that today.

ifitzsimmons · 2021-01-05T17:22:53Z

@pgrzesik I got dinged for a dip in code coverage for the following changes... can we override that github action?

I ran coverage locally for these files and nothing has changed between master and this branch as a sanity check.

pgrzesik

Thank you @ifitzsimmons for the changes. I've inspected the coverage changes and it's possible that I gave you the wrong suggestion with dropping the async from extendedValidate. It is used in a few places and possibly it's expected from that function to return a Promise (it's used in AwsInvoke where we see the dropped coverage). Would you be able to change it back and validate again if we're experiencing coverage drop? Sorry for the trouble 🙇

pgrzesik · 2021-01-06T10:02:37Z

test/unit/lib/plugins/aws/deploy/lib/createStack.test.js

@@ -126,11 +126,11 @@ describe('createStack', () => {
    it('should throw error if invalid stack name', () => {
      sandbox.stub(awsDeploy, 'create').resolves();
      sandbox.stub(awsDeploy.provider, 'request').resolves();
-      awsDeploy.serverless.service.service = 'service-name'.repeat(100);
+      awsDeploy.serverless.service.provider.stackName = 'service-name'.repeat(100);


Is there a reason to change this test setup here?

Huh, I'm not sure how or why that snuck in there.... I did perform a rebase, but to answer your question - no, there's no reason that that should be there.

ifitzsimmons · 2021-01-07T03:08:04Z

Thank you @ifitzsimmons for the changes. I've inspected the coverage changes and it's possible that I gave you the wrong suggestion with dropping the async from extendedValidate. It is used in a few places and possibly it's expected from that function to return a Promise (it's used in AwsInvoke where we see the dropped coverage). Would you be able to change it back and validate again if we're experiencing coverage drop? Sorry for the trouble 🙇

I think that maybe I caused that confusion. I removed all async/await logic associated with extendedValidate(). Before these changes, it returned BBPromise.resolve() without ever calling any functions that returned promises. I think your initial recommendation to remove async from extended validate was spot on. Still having codecov issues since I removed the call to BBPromise.resolve. Adding a test case now to increase codecov

ifitzsimmons · 2021-01-07T03:41:09Z

@pgrzesik Am I missing something here. When I click on the codecov report, I am seeing that extendedValidate() is the violator. But when I run codecoverage locally, my branch has more code coverage than when I run coverage locally on master.

My branch is on the left, master is on the right.

pgrzesik · 2021-01-07T11:55:41Z

I went through it once again and I believe it's just coverage playing with us here, it looks like it's all good, however, I think we should remove the last change with { image: true } as it doesn't actually test anything extra as there are no new assertions in the test. Thanks for being patient with that PR 🙇

ifitzsimmons · 2021-01-07T21:09:00Z

I went through it once again and I believe it's just coverage playing with us here, it looks like it's all good, however, I think we should remove the last change with { image: true } as it doesn't actually test anything extra as there are no new assertions in the test. Thanks for being patient with that PR 🙇

I'm happy to remove it, but it does add coverage. I added another function object, but no additional calls to fileExistsSyncStub were made because of the following line

if (functionObject.image) return;

If that object did not have an image property, we'd have to change the line in the test case:

- expect(fileExistsSyncStub.calledTwice).to.equal(true);
+ expect(fileExistsSyncStub.calledThrice).to.equal(true);

Let me know if you still want me to remove that line!! I'm happy to do so

pgrzesik · 2021-01-08T08:50:21Z

Hey @ifitzsimmons 👋 I understand that it might add coverage there, but the definition of that image is invalid (currently it expects a valid Docker Image URI) and there are no extra assertions added, so there's little value in added coverage if we don't really test the outcome of the tested path. I'll say let's remove the whole

second: {
  image: true,
},

that was added, I don't think it will require changes in the assertions

ifitzsimmons · 2021-01-18T22:17:59Z

@pgrzesik Are you waiting on me for anything for this PR?

pgrzesik · 2021-01-19T09:12:14Z

Hello @ifitzsimmons, really sorry about it - I didn't realise that you pushed new changes because I'm monitoring review requests and comments via notifications and didn't get anything :( The best bet to ensure I or someone else sees the changes pushed is to request for re-review after completing changes.

Unfortunately, I see that there's one extra conflicting file that will need to be resolved before we can merge this one

Refactored Promise-returning functions for: * `lib/plugin/aws/customResources` * `lib/plugin/aws/deploy`

ifitzsimmons · 2021-01-19T19:14:45Z

@pgrzesik That codecov issue resurfaced, can we ignore it again?

pgrzesik

Awesome job 🙌 Coverage looks good, your diff has 100% of coverage 💯

ifitzsimmons force-pushed the 8369-async-refactor branch from 49a406c to d3cd92e Compare January 1, 2021 23:19

pgrzesik suggested changes Jan 4, 2021

View reviewed changes

ifitzsimmons force-pushed the 8369-async-refactor branch from d37f96c to ebdcb74 Compare January 5, 2021 16:39

pgrzesik suggested changes Jan 6, 2021

View reviewed changes

ifitzsimmons force-pushed the 8369-async-refactor branch from 9272f8b to 8a6689b Compare January 8, 2021 18:35

8368: Configure all promise returning functions as async functions

9a7565e

Refactored Promise-returning functions for: * `lib/plugin/aws/customResources` * `lib/plugin/aws/deploy`

ifitzsimmons force-pushed the 8369-async-refactor branch from 8a6689b to 9a7565e Compare January 19, 2021 19:07

ifitzsimmons requested a review from pgrzesik January 19, 2021 19:13

pgrzesik approved these changes Jan 19, 2021

View reviewed changes

pgrzesik changed the title ~~8368: Configure all promise returning functions as async functions~~ refactor: Mark functions async in aws/customResources and aws/deploy Jan 19, 2021

pgrzesik merged commit c45f661 into serverless:master Jan 19, 2021

pgrzesik mentioned this pull request Jan 20, 2021

fix: Complete async refactor for aws/deploy and customResources #8792

Merged

pgrzesik mentioned this pull request Feb 8, 2021

Configure all promise returning functions as async functions #8368

Closed

43 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Mark functions async in `aws/customResources` and `aws/deploy` #8698

refactor: Mark functions async in `aws/customResources` and `aws/deploy` #8698

ifitzsimmons commented Jan 1, 2021

codecov bot commented Jan 1, 2021 •

edited

pgrzesik left a comment

pgrzesik Jan 4, 2021

pgrzesik Jan 4, 2021

ifitzsimmons Jan 4, 2021

pgrzesik Jan 4, 2021

pgrzesik Jan 4, 2021

pgrzesik Jan 4, 2021

ifitzsimmons commented Jan 5, 2021

pgrzesik commented Jan 5, 2021

ifitzsimmons commented Jan 5, 2021

ifitzsimmons commented Jan 5, 2021

pgrzesik left a comment

pgrzesik Jan 6, 2021

ifitzsimmons Jan 6, 2021

ifitzsimmons commented Jan 7, 2021

ifitzsimmons commented Jan 7, 2021 •

edited

pgrzesik commented Jan 7, 2021

ifitzsimmons commented Jan 7, 2021

pgrzesik commented Jan 8, 2021 •

edited

ifitzsimmons commented Jan 18, 2021

pgrzesik commented Jan 19, 2021

ifitzsimmons commented Jan 19, 2021

pgrzesik left a comment

refactor: Mark functions async in aws/customResources and aws/deploy #8698

refactor: Mark functions async in aws/customResources and aws/deploy #8698

Conversation

ifitzsimmons commented Jan 1, 2021

codecov bot commented Jan 1, 2021 • edited

Codecov Report

pgrzesik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ifitzsimmons commented Jan 5, 2021

pgrzesik commented Jan 5, 2021

ifitzsimmons commented Jan 5, 2021

ifitzsimmons commented Jan 5, 2021

pgrzesik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ifitzsimmons commented Jan 7, 2021

ifitzsimmons commented Jan 7, 2021 • edited

pgrzesik commented Jan 7, 2021

ifitzsimmons commented Jan 7, 2021

pgrzesik commented Jan 8, 2021 • edited

ifitzsimmons commented Jan 18, 2021

pgrzesik commented Jan 19, 2021

ifitzsimmons commented Jan 19, 2021

pgrzesik left a comment

Choose a reason for hiding this comment

refactor: Mark functions async in `aws/customResources` and `aws/deploy` #8698

refactor: Mark functions async in `aws/customResources` and `aws/deploy` #8698

codecov bot commented Jan 1, 2021 •

edited

ifitzsimmons commented Jan 7, 2021 •

edited

pgrzesik commented Jan 8, 2021 •

edited