Skip to content
This repository has been archived by the owner on Mar 11, 2024. It is now read-only.

Add cleanup for new block stream #82

Merged
merged 10 commits into from Mar 15, 2019

Conversation

adrianmcli
Copy link
Contributor

Fixes #81

})
.on("error", err => subscriber.next(err));
});
const observable = new Subject();
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason for changing to Subject? I seem to recall an earlier discussion that we had on changing the Subjects to Observables, since you couldn't push your own events to an Observable it was safer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You need the Subject to be able to call .complete on it outside of the function. I suppose you could assign it to a temp variable by using its closure.

@honestbonsai
Copy link
Contributor

@adrianmcli CI is failing

@adrianmcli
Copy link
Contributor Author

For some reason, using Subject for fromPolling makes the tests hang so I have reverted to using Observable for now. But this also means that the stream doesn't automatically complete when cleanup() is called.

This might cause problems down the road, but I'm not sure what we can do at this point. I'm not sure why using Subject causes it to hang.

btw, when I say "hang", I mean getting the following at the end of successfully passing all tests:

Jest did not exit one second after the test run has completed.

This usually means that there are asynchronous operations that weren't stopped in your tests. Consider running Jest with `--detectOpenHandles` to troubleshoot this issue.

No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.

Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received

Do note that this only happens on Travis, not locally.

@adrianmcli
Copy link
Contributor Author

@honestbonsai I found this comment: jestjs/jest#1456 (comment)

And I basically just set maxWorkers to a high number (10 in this case) and then the test seems to pass. If this is satisfactory to you, then I think we should merge. If not, then feel free to revert to commit 69d3e54 and merge that instead. I'll leave it up to you since I'll be in the air tomorrow.

@honestbonsai
Copy link
Contributor

honestbonsai commented Mar 14, 2019

Hmm still broken, I'll take a closer look if you're busy. @adrianmcli

@adrianmcli
Copy link
Contributor Author

@honestbonsai I triggered a rebuild and it passed. It just didn't update on github.

@honestbonsai
Copy link
Contributor

Ok that works for me then

@honestbonsai honestbonsai merged commit cf5592b into master Mar 15, 2019
@honestbonsai honestbonsai deleted the chore/add-cleanup-for-new-block-stream branch March 15, 2019 18:33
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants