Close all handles during Jest teardown #2330

luckysori · 2020-03-26T11:18:43Z

Waiting for comit-network/comit-js-sdk#176 to be merged and for those changes to be included in a release of comit-js-sdk before we can update the last commit in this PR to use that release instead of a local dependency created via yalc (which seems to work much better than yarn link).

Fixes #2211.

Turns out Jest will hang until every timeout is cleared, so racing against a timeout that is only cleared after it resolves was not good.
The proper teardown of the e2e test environment is fully achieved with 3698f3a, after a lot of debugging. Unfortunately, Jest recommends this

Consider running Jest with `--detectOpenHandles` to troubleshoot this issue

when tests hang after passing, but it just doesn't seem to do anything. I tried using leaked-handles without success, but finally found wtfnode, which identified bcoin as the culprit.

My OCD was definitely triggered with this one, as a different error got me to investigate how to get rid of smack-my-jasmine-up. Turns out removing it did not fix that other error, but I still see it as a win, since depending on such an obscure library is bad.

And another bug squashed! No more

MaxListenersExceededWarning: Possible EventEmitter memory leak detected. 11 SIGHUP listeners added to [process]. Use emitter.setMaxListeners() to increase limit

errors, by combining cde8e0c and 127a4a2:

shutting down the logger during teardown.
closing log files after 5 seconds of inactivity, to prevent us from reaching the limit of listeners.

I believe log files can be reopened if something else needs to be logged.

mergify · 2020-03-26T11:19:16Z

Are you sure the changelog does not need updating?

mergify · 2020-03-26T11:19:16Z

Are you sure the changelog does not need updating?

mergify · 2020-03-26T11:19:17Z

Are you sure the changelog does not need updating?

bonomat · 2020-03-26T21:55:20Z

smack-my-jasmine-up always annoyed me a bit aswell.
So what's your strategy here now as your PR does not seem to build?

luckysori · 2020-03-26T22:10:06Z

smack-my-jasmine-up always annoyed me a bit aswell.
So what's your strategy here now as your PR does not seem to build?

Silly me, forgot to do the same for e2e tests (not just dry)!

thomaseizinger

You are a genius Lucas :)
Go job figuring that one out!

thomaseizinger · 2020-03-26T23:01:15Z

api_tests/src/utils.ts

            reject(new Error(`timed out after ${ms}ms`));
        }, ms);
    });

    // Returns a race between our timeout and the passed in promise
-    return Promise.race([promise, timeout]);
+    return Promise.race([promise, timeout]).then((result) => {


Should we maybe use .finally here, just to be sure?

thomaseizinger · 2020-03-29T23:50:42Z

api_tests/src/configure_jasmine.ts

@@ -0,0 +1,5 @@
+jasmine.getEnv().addReporter({
+    specStarted: (result) =>
+        // @ts-ignore


Can we get typings for this instead of using ts-ignore?

Maybe you can help me figure this one out.

I now realise that I'm using the jasmine namespace to set and read a value which wasn't there in the first place i.e. there is no type because I'm creating that "field".

Would it make more sense to save this to the global context of the e2e test suite?

Would it make more sense to save this to the global context of the e2e test suite?

I guess not because this global context can be accessed by two tests running concurrently.

I'm just gonna leave this one as an optional follow-up PR. Had a look at merging namespaces to extend the one provided by jest with the type we need here, but there's probably a much easier approach that I'm missing!

I guess not because this global context can be accessed by two tests running concurrently.

The test environment provides a unique instance of global to every test so you can safely set things there!

But yes, we can do this as a follow-up. I don't have time at the moment so you could write a short issue please? :)

api_tests/src/e2e_test_environment.ts

api_tests/src/utils.ts

thomaseizinger · 2020-03-29T23:57:59Z

So many good fixes in this one :)

176: Add close method for bcoin wallet r=mergify[bot] a=luckysori These are a couple of patches that need to be released before comit-network/comit-rs#2330 can be merged in. Co-authored-by: Lucas Soriano del Pino <l.soriano.del.pino@gmail.com>

api_tests/package.json

`yalc` seems more reliable than `yarn link`.

This allows us to remove the weird `smack-my-jasmine-up` dependency.

We ensure that all nodes are stopped and all wallets are closed during Jest teardown.

To prevent reaching the limit of listeners.

bonomat

Good job.

bonomat · 2020-03-30T20:04:52Z

bors r+

mergify · 2020-03-30T20:05:10Z

bors r+

bors · 2020-03-30T20:05:11Z

Already running a review

bors · 2020-03-30T21:14:11Z

Build succeeded

ci/circleci: debug-build-test

Motivated by the combination of wanting to test `tryExecuteSirenAction` and realising that we would just be testing `timeoutPromise`. We have already decided to replace exactly the same function in `comit-rs` (comit-network/comit-rs#2330 (comment)), so replacing it here has the nice consequence of not needing to write a new test, since it's not our own code. Side-effects: - Move `TryParams` to `swap.ts` since it's directly related and only used for executing a swap. - Rename `util/timeout_promise.ts` to `util/sleep.ts`, since it now only holds a `sleep` function.

179: Replace timeout function with external dependency r=mergify[bot] a=luckysori Fixes #120 (imo). ## Straight from the commit Motivated by the combination of wanting to test `tryExecuteSirenAction` and realising that we would just be testing `timeoutPromise`. We have already decided to replace exactly the same function in `comit-rs` (comit-network/comit-rs#2330 (comment)), so replacing it here has the nice consequence of not needing to write a new test, since it's not our own code. Side-effects: - Move `TryParams` to `swap.ts` since it's directly related and only used for executing a swap. - Rename `util/timeout_promise.ts` to `util/sleep.ts`, since it now only holds a `sleep` function. ## Notes I don't believe this requires a changelog update. Am I wrong? Co-authored-by: Lucas Soriano del Pino <l.soriano.del.pino@gmail.com>

luckysori requested review from bonomat and thomaseizinger March 26, 2020 11:18

github-actions bot assigned luckysori Mar 26, 2020

bonomat mentioned this pull request Mar 26, 2020

Changelog bot is out of control #2333

Closed

thomaseizinger reviewed Mar 26, 2020

View reviewed changes

luckysori mentioned this pull request Mar 28, 2020

Add close method for bcoin wallet comit-network/comit-js-sdk#176

Merged

luckysori added the blocked label Mar 28, 2020

luckysori changed the title ~~Clear timeout even if race resolves before timeout~~ Close all handles during Jest teardown Mar 29, 2020

bonomat added the no-mergify Stop mergify to merge this automatically label Mar 29, 2020

luckysori requested a review from thomaseizinger March 29, 2020 23:45

thomaseizinger approved these changes Mar 29, 2020

View reviewed changes

bonomat reviewed Mar 30, 2020

View reviewed changes

api_tests/package.json Outdated Show resolved Hide resolved

luckysori removed the blocked label Mar 30, 2020

luckysori force-pushed the 2211-graceful-exit branch from 127a4a2 to b90d60f Compare March 30, 2020 03:20

luckysori added 6 commits March 30, 2020 14:21

Add yalc files to .gitignore

e4e4ab3

`yalc` seems more reliable than `yarn link`.

Replace timeout function with external dependency

b794b08

Configure Jasmine to report current test name

6f24ff2

This allows us to remove the weird `smack-my-jasmine-up` dependency.

Close all blockchain-related handles during Jest teardown

9f75317

We ensure that all nodes are stopped and all wallets are closed during Jest teardown.

Shut down logger during Jest teardown

251c44f

Close log files after 5 seconds of inactivity

3fb3ffb

To prevent reaching the limit of listeners.

luckysori force-pushed the 2211-graceful-exit branch from b90d60f to 3fb3ffb Compare March 30, 2020 03:29

luckysori removed the no-mergify Stop mergify to merge this automatically label Mar 30, 2020

luckysori requested a review from bonomat March 30, 2020 03:30

bonomat approved these changes Mar 30, 2020

View reviewed changes

bors bot merged commit 23dda65 into dev Mar 30, 2020

This was referenced Mar 31, 2020

Replace timeout function with external dependency comit-network/comit-js-sdk#179

Merged

Parallel e2e tests: Only start one instance of Parity, Bitcoind and one lnd per actor #2289

Merged

thomaseizinger deleted the 2211-graceful-exit branch April 26, 2020 06:11

luckysori mentioned this pull request Apr 28, 2020

Remove ts-ignores introduced by #2330 #2352

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Close all handles during Jest teardown #2330

Close all handles during Jest teardown #2330

luckysori commented Mar 26, 2020 •

edited

mergify bot commented Mar 26, 2020

mergify bot commented Mar 26, 2020

mergify bot commented Mar 26, 2020

bonomat commented Mar 26, 2020

luckysori commented Mar 26, 2020

thomaseizinger left a comment

thomaseizinger Mar 26, 2020

thomaseizinger Mar 29, 2020

luckysori Mar 30, 2020

luckysori Mar 30, 2020

luckysori Mar 30, 2020

thomaseizinger Mar 30, 2020

thomaseizinger Mar 30, 2020

thomaseizinger commented Mar 29, 2020

bonomat left a comment

bonomat commented Mar 30, 2020

mergify bot commented Mar 30, 2020

bors bot commented Mar 30, 2020

bors bot commented Mar 30, 2020

Close all handles during Jest teardown #2330

Close all handles during Jest teardown #2330

Conversation

luckysori commented Mar 26, 2020 • edited

mergify bot commented Mar 26, 2020

mergify bot commented Mar 26, 2020

mergify bot commented Mar 26, 2020

bonomat commented Mar 26, 2020

luckysori commented Mar 26, 2020

thomaseizinger left a comment

Choose a reason for hiding this comment

thomaseizinger Mar 26, 2020

Choose a reason for hiding this comment

thomaseizinger Mar 29, 2020

Choose a reason for hiding this comment

luckysori Mar 30, 2020

Choose a reason for hiding this comment

luckysori Mar 30, 2020

Choose a reason for hiding this comment

luckysori Mar 30, 2020

Choose a reason for hiding this comment

thomaseizinger Mar 30, 2020

Choose a reason for hiding this comment

thomaseizinger Mar 30, 2020

Choose a reason for hiding this comment

thomaseizinger commented Mar 29, 2020

bonomat left a comment

Choose a reason for hiding this comment

bonomat commented Mar 30, 2020

mergify bot commented Mar 30, 2020

bors bot commented Mar 30, 2020

bors bot commented Mar 30, 2020

Build succeeded

luckysori commented Mar 26, 2020 •

edited