Promise all #447

king-11 · 2020-10-05T07:22:59Z

Description

Update sequentially running loops for promise resolution to concurrently running promises using Promise.all while updating tests for two functions as promises are now run concurrently.

Motivation and Context

promise resolution sequentially takes a lot of time to resolve all when then can instead run concurrently.

closes #287

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

I have updated the documentation accordingly.
I have added new practice to practice list in README.md.
I have read the CONTRIBUTING document.
I haven't repeated the code. (DRY)
I have added tests to cover my changes.
All new and existing tests passed.

adelkahomolova · 2020-10-05T07:39:41Z

Hi, @king-11 there is one test failing. Could you look at it, please? If you'll need any help, let me now. It seems it's just the wrong use of nock (which is not hard to use it badly actually...).

king-11 · 2020-10-05T07:42:19Z

@adelkahomolova is HTTP the expected error? if yes then I shall make amends in the test to check for it by seeing if the error name is HttpError or something else would be better

adelkahomolova · 2020-10-05T08:20:24Z

No, this error is not expected. It tells us just the fact that the nock request does not correspond with the request which is made in git.flatTraverse() method. Maybe some of the calls are not executed. 🤔 @king-11

king-11 · 2020-10-05T11:07:40Z

@adelkahomolova as you said some calls are not executed its coz one rejection in Promise.all stops others so if that has to be prevented I changed it to promise.allSettled and then some to check for rejection

adelkahomolova · 2020-10-05T13:12:22Z

Hey, you shouldn't change the code just because of the test.
Please, keep your code in Git.ts and change the test. I think that just remove the calling second nock should help. It should fail fast in this case. I can explain it more, why we don't want to use Promise.allSettled() if you will need it or want it :) @king-11

king-11 · 2020-10-05T13:29:40Z

@adelkahomolova ahh sorry 😅 I am not much aware abt nock calls so i thought this can help but i will try as you say 👍 and Yeah, I would also like to hear why Promise.allSettled isn't correct.

I tried tracing the error that was thrown it was from src/services/git/GitHubService.ts:332:41
on line

try {
        response = <GetContentsResponse>await this.unwrap(this.client.repos.getContent({ owner, repo, path }));
       } catch (e) {
         if (e.name !== 'HttpError' || e.status !== 404) {
         throw e;
     }

adelkahomolova · 2020-10-05T13:36:23Z

The problem is, that you reject the whole array of promises after all, even if there is just one call that you rejected. If you will use Promise.allSettled it will make a call for every single item in the array, even if it hits a snag. Then you will take a whole array and check if one of the items was rejected. If yes you'll reject the whole array. So it takes a lot of time. But if you will use the Promise.all(), it is rejected right after getting the first rejection.
I hope I explain it understandable 😄 . Ask me, if you have any questions. :) @king-11

king-11 · 2020-10-05T13:43:53Z

@adelkahomolova yeah understood this that time is an issue while using allSettled.
so i changed test as follows and its passing now does it look good to you ?

it('stops on false', async () => {
      gitHubNock.getDirectory('mockFolder', ['mockFile.ts'], ['mockSubFolder']);
      gitHubNock.getFile('mockFolder/mockFile.ts');
      gitHubNock.getDirectory('mockFolder/mockSubFolder', ['mockSubFolderFile.txt'], []);
      gitHubNock.getFile('mockFolder/mockSubFolder/mockSubFolderFile.txt');

      let response = true;
      await git
        .flatTraverse('mockFolder', () => {
          return false;
        })
        .catch((e) => (response = e));

      expect(response).toEqual(false);
    });

adelkahomolova · 2020-10-05T14:10:51Z

Try something like this so we use DRY principle. @king-11 Thanks!

king-11 · 2020-10-05T18:57:02Z

@adelkahomolova for DRY principle, I was thinking we can use beforeEach and have a nested describe inside flat traverse which can use beforeEach and instantiate the directories because there are two tests that need complete instantiation so something like this

describe('#fetch dir and files', () => {
      beforeEach(() => {
        gitHubNock.getDirectory('mockFolder', ['mockFile.ts'], ['mockSubFolder']);
        gitHubNock.getFile('mockFolder/mockFile.ts');
        gitHubNock.getDirectory('mockFolder/mockSubFolder', ['mockSubFolderFile.txt'], []);
        gitHubNock.getFile('mockFolder/mockSubFolder/mockSubFolderFile.txt');
      });
      it('returns keys of metadata of all results', async () => {
        const files: string[] = [];

        await git.flatTraverse('mockFolder', (meta) => {
          files.push(meta.name);
        });

        expect(files.length).toEqual(3);
        expect(files).toContain('mockFile.ts');
        expect(files).toContain('mockSubFolder');
        expect(files).toContain('mockSubFolderFile.txt');
      });

      it('stops on false', async () => {
        let response = true;
        await git
          .flatTraverse('mockFolder', () => {
            return false;
          })
          .catch((e) => (response = e));

        expect(response).toEqual(false);
      });
    });

prokopsimek · 2020-10-06T05:37:27Z

@king-11 What about to change the catch to:

    it('stops on false', async () => {
        let response = true;
        await git
          .flatTraverse('mockFolder', () => {
            return false;
          })
          .catch((e) => (fail(response)));

        expect(response).toEqual(false);
      });

?

I am fine with your solution. We could fail the test with a relevant error message with jest's fail function.

king-11 · 2020-10-06T05:39:59Z

@prokopsimek didn't get it completely but I suppose you want to ensure that we use something specifically to check function fail

it('stops on false', async () => {
        await expect(
          git.flatTraverse('mockFolder', () => {
            return false;
          }),
        ).rejects.toBe(false);
      });

I was thinking maybe something like this because I wasn't able to understand the fail func use case here 🤔 because fail checks for error thrown i guess so i will have to make amends in git.flattraverse() then to throw error instead of rejecting promise with false value

adelkahomolova · 2020-10-06T07:10:16Z

I think @prokopsimek thinks that your solution is good. He just wanted to fail the test with a relevant message, so you should just change (response = e) for (fail(response)). @king-11

king-11 · 2020-10-06T11:37:16Z

@adelkahomolova catch is the place through which am setting value for a response as false. If I change that then the response won't get a false value and will fail even though e returned false 😅 so I got the point about using fail so it can be something like this :

it('stops on false', async () => {
        await git
          .flatTraverse('mockFolder', () => {
            return false;
          })
          .then(() => fail("promise didn't fail even on false return"))
          .catch((e) => expect(e).toBe(false));
      });

so here if no promise was rejected then it will fail else will pass.

king-11 · 2020-10-06T12:50:13Z

@adelkahomolova still to push the changes that i cited just a sec 😅

adelkahomolova · 2020-10-06T12:52:24Z

Great, this should work. So when you'll implement it, I'll merge it. 🎉
I really appreciate the time you invest in the dx-scanner. I hope you enjoyed contributing to our repo! I'll be glad if you'll let us some feedback either for communication and for the dx-scanner itself. Every thought is valuable for us. Thanks, @king-11

check for false in promise reject for test Promise.all resolves all promises concurrently

resolve promise concurrently and then spread in languageContext.ts scanner.ts return promise resolved by Promise.all

options object maybe undefined hence use optional chaining Promise all to run functions concurrently

check for false in promise reject for test Promise.all resolves all promises concurrently

change to Promise.allSettled coz each https request has to be completed loop to check if rejection then return promise reject false lodash unneeded import by auto import removed

fail function to fail test with proper error make code more concise

king-11 · 2020-10-06T13:08:25Z

rebased with upstream and added changes and @adelkahomolova @prokopsimek thanks a lot really for helping out throughout this is my first major contribution and thanks to you guys I received a lot of help and really enjoyed contributing to dx-scanner will continue doing that as its a really great tool. and run failed :'( api rate limit again

adelkahomolova · 2020-10-06T13:11:50Z

I'm glad to hear that! ❤️
I know, the API rate limit is the issue we're trying to solve. Sorry for that 😢

prokopsimek · 2020-10-07T11:40:48Z

🎉 This PR is included in version 3.40.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

king-11 requested review from adelkahomolova, Jakub-Vacek, prokopsimek and ryzzaki as code owners October 5, 2020 07:22

king-11 force-pushed the promise-all branch from 081dfca to e809d95 Compare October 5, 2020 11:29

adelkahomolova approved these changes Oct 6, 2020

View reviewed changes

king-11 added 6 commits October 6, 2020 18:24

Git.ts update flat traverse and its test

e745c47

check for false in promise reject for test Promise.all resolves all promises concurrently

resolve promises concurrent scanner and languageContext

57299cf

resolve promise concurrently and then spread in languageContext.ts scanner.ts return promise resolved by Promise.all

promise all and optional chaining update FileInspector.ts

0473d41

options object maybe undefined hence use optional chaining Promise all to run functions concurrently

FileSystemService.ts update flat traverse and its test

5765ad5

check for false in promise reject for test Promise.all resolves all promises concurrently

remove unnecessary import resolve error

1fdbdd4

change to Promise.allSettled coz each https request has to be completed loop to check if rejection then return promise reject false lodash unneeded import by auto import removed

fail function use and removing unnecessary variable

febdf71

fail function to fail test with proper error make code more concise

king-11 force-pushed the promise-all branch from e809d95 to febdf71 Compare October 6, 2020 12:54

adelkahomolova merged commit d07bc9f into DXHeroes:master Oct 7, 2020

prokopsimek added the released label Oct 7, 2020

king-11 deleted the promise-all branch October 7, 2020 13:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Promise all #447

Promise all #447

king-11 commented Oct 5, 2020

adelkahomolova commented Oct 5, 2020

king-11 commented Oct 5, 2020 •

edited

adelkahomolova commented Oct 5, 2020

king-11 commented Oct 5, 2020 •

edited

adelkahomolova commented Oct 5, 2020

king-11 commented Oct 5, 2020 •

edited

adelkahomolova commented Oct 5, 2020

king-11 commented Oct 5, 2020 •

edited

adelkahomolova commented Oct 5, 2020

king-11 commented Oct 5, 2020 •

edited

prokopsimek commented Oct 6, 2020 •

edited

king-11 commented Oct 6, 2020 •

edited

adelkahomolova commented Oct 6, 2020

king-11 commented Oct 6, 2020 •

edited

king-11 commented Oct 6, 2020

adelkahomolova commented Oct 6, 2020

king-11 commented Oct 6, 2020

adelkahomolova commented Oct 6, 2020

prokopsimek commented Oct 7, 2020

Promise all #447

Promise all #447

Conversation

king-11 commented Oct 5, 2020

Description

Motivation and Context

Types of changes

Checklist:

adelkahomolova commented Oct 5, 2020

king-11 commented Oct 5, 2020 • edited

adelkahomolova commented Oct 5, 2020

king-11 commented Oct 5, 2020 • edited

adelkahomolova commented Oct 5, 2020

king-11 commented Oct 5, 2020 • edited

adelkahomolova commented Oct 5, 2020

king-11 commented Oct 5, 2020 • edited

adelkahomolova commented Oct 5, 2020

king-11 commented Oct 5, 2020 • edited

prokopsimek commented Oct 6, 2020 • edited

king-11 commented Oct 6, 2020 • edited

adelkahomolova commented Oct 6, 2020

king-11 commented Oct 6, 2020 • edited

king-11 commented Oct 6, 2020

adelkahomolova commented Oct 6, 2020

king-11 commented Oct 6, 2020

adelkahomolova commented Oct 6, 2020

prokopsimek commented Oct 7, 2020

king-11 commented Oct 5, 2020 •

edited

king-11 commented Oct 5, 2020 •

edited

king-11 commented Oct 5, 2020 •

edited

king-11 commented Oct 5, 2020 •

edited

king-11 commented Oct 5, 2020 •

edited

prokopsimek commented Oct 6, 2020 •

edited

king-11 commented Oct 6, 2020 •

edited

king-11 commented Oct 6, 2020 •

edited