fix: handle throw error inside retryTo promise

[Horsty80]

Motivation/Description of the PR

• When a throw error occurs inside promise, that stale the process, by rewrite with good reject and done call, we prevent this to happen

• 4367

• maybe this pr can help this issue #4358

Applicable plugins:

• [x] retryTo

Type of change

• [x] :bug: Bug fix

Checklist:

• [x] Tests have been added
• [x] Local tests are passed (Run npm test)

[Horsty80]

@kobenguyent with a friend, we have work on this, you can find a reproducible unit test that lead to a stale process without our change on retryTo.js

It's seems not terminating promises properly can lead to a stale process. So maybe a clue to resolve issue #4358

Let me know if this needs some reworks

Thanks

[kobenguyent]

Hey @Horsty80 thanks for the investigation. May you hell check the UTs? Failed tests there. 🤔

[Horsty80]

Hey @Horsty80 thanks for the investigation. May you hell check the UTs? Failed tests there. 🤔

I check the why it's failing, on my laptop the npm run test is working and correct that

[Horsty80]

@kobenguyent TU are fix now :)

[kobenguyent]

Could you do me a favor please?

Trying your fix with scenario listed here. https://github.com/codeceptjs/CodeceptJS/issues/4197

If that the case then I don't think my fix is needed or perhaps an extra layer to be sure.

[Horsty80]

Could you do me a favor please?

Trying your fix with scenario listed here. #4197

If that the case then I don't think my fix is needed or perhaps an extra layer to be sure.

I've check the scenario

Scenario('test issue', async ({ I }) => {
    I.amOnPage('http://example.org')
    I.waitForVisible('.nothing', 1); // should fail here but it won't terminate
    await retryTo( (tryNum) => {
        I.see(".doesNotMatter");
    }, 10);
});

And I think is another issue, not on the retryTo I'm working on fix this to so I will fix it in another PR

I've tested this scenario and the issue (without your fix) is still present. Sequence I.waitForVisible that failed and after a retryTo lead to stale the process

[kobenguyent]

Could you do me a favor please? Trying your fix with scenario listed here. #4197 If that the case then I don't think my fix is needed or perhaps an extra layer to be sure.

I've check the scenario

Scenario('test issue', async ({ I }) => {
    I.amOnPage('http://example.org')
    I.waitForVisible('.nothing', 1); // should fail here but it won't terminate
    await retryTo( (tryNum) => {
        I.see(".doesNotMatter");
    }, 10);
});

And I think is another issue, not on the retryTo I'm working on fix this to so I will fix it in another PR

I've tested this scenario and the issue (without your fix) is still present. Sequence I.waitForVisible that failed and after a retryTo lead to stale the process

Thanks @Horsty80. I guess that aforementioned covered by https://github.com/codeceptjs/CodeceptJS/pull/4367

[Horsty80]

Could you do me a favor please? Trying your fix with scenario listed here. #4197 If that the case then I don't think my fix is needed or perhaps an extra layer to be sure.

I've check the scenario

Scenario('test issue', async ({ I }) => {
    I.amOnPage('http://example.org')
    I.waitForVisible('.nothing', 1); // should fail here but it won't terminate
    await retryTo( (tryNum) => {
        I.see(".doesNotMatter");
    }, 10);
});

And I think is another issue, not on the retryTo I'm working on fix this to so I will fix it in another PR I've tested this scenario and the issue (without your fix) is still present. Sequence I.waitForVisible that failed and after a retryTo lead to stale the process

Thanks @Horsty80. I guess that aforementioned covered by #4367

Indeed maybe your fix cover this case.

[kobenguyent]

as this also addresses the stale process, so I think we would combine them into a branch. Then provide a beta version to test it out.