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

tests: wait for multisig tx to arrive before continuing the test #308

Merged
merged 4 commits into from
Jul 4, 2023

Conversation

pedroferreira1
Copy link
Member

@pedroferreira1 pedroferreira1 commented Jun 20, 2023

Issue: #292

Acceptance Criteria

  • Wait multisig transaction before continuing test to prevent out of funds error

Security Checklist

  • Make sure you do not include new dependencies in the project unless strictly necessary and do not include dev-dependencies as production ones. More dependencies increase the possibility of one of them being hijacked and affecting us.

@codecov
Copy link

codecov bot commented Jun 20, 2023

Codecov Report

Merging #308 (0978338) into dev (4a8f508) will not change coverage.
The diff coverage is n/a.

❗ Current head 0978338 differs from pull request most recent head affaff7. Consider uploading reports for the commit affaff7 to get more accurate results

@@           Coverage Diff           @@
##              dev     #308   +/-   ##
=======================================
  Coverage   86.98%   86.98%           
=======================================
  Files          35       35           
  Lines        1352     1352           
  Branches      269      269           
=======================================
  Hits         1176     1176           
  Misses        155      155           
  Partials       21       21           

@pedroferreira1 pedroferreira1 force-pushed the tests/integration-tests-no-funds branch from 4dee570 to f9be24a Compare June 22, 2023 19:43
}, timeout);
}

while ((await TestUtils.getTransaction(walletId, txId)).success === false) {
Copy link
Contributor

Choose a reason for hiding this comment

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

When a timeout is passed as parameter, I believe this while block will continue running even after a reject is sent. ( similar case on a SO question )

Since this is an util used only on the integration tests, the effects of these "leaks" will probably not be very impactful, but I think it's important to at least document this behavior, so that future improvements can target this easily.

Copy link
Member Author

Choose a reason for hiding this comment

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

From the SO answer I read this:

Unless an error is thrown, a function is executed until a return statement or its end is reached. Other code outside of the function can't interfere with that (unless, again, an error is thrown).

So if I just add the return reject it should be enough, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, this is not enough because the return is ending the setTimeout block, I've done some tests here and double-checked it.

I've done the timeout handling a bit differently, what do you think? 0978338

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's a good solution 👍

@pedroferreira1 pedroferreira1 force-pushed the tests/integration-tests-no-funds branch from 0978338 to affaff7 Compare July 4, 2023 02:18
@pedroferreira1 pedroferreira1 requested a review from tuliomir July 4, 2023 02:42
@pedroferreira1 pedroferreira1 merged commit e3712ea into dev Jul 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants