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

Fix test issue as reported in #195 #196

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

nordfjord
Copy link
Collaborator

The issue came about because the test is asynchronous but wasn't
supplied a done callback.

This lead to a swallowed error.

This commit fixes the test such that it gives the expected result and
calls the done callback

The issue came about because the test is asynchronous but wasn't
supplied a done callback.

This lead to a swallowed error.

This commit fixes the test such that it gives the expected result and
calls the done callback
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 35cb544 on fix/chain-test into 180e8f5 on master.

@coveralls
Copy link

coveralls commented Mar 29, 2019

Coverage Status

Coverage remained the same at 100.0% when pulling 3e3dfca on fix/chain-test into 180e8f5 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 35cb544 on fix/chain-test into 180e8f5 on master.

@StreetStrider
Copy link
Contributor

From my perspective, done callback is a mess and it's not unusual to forget it.

I always settle my system, defining something like drain(Stream<T>) → Promise<T>, then making all tests async and return drain(s) from them, where s is (in some sence) considered main instance of that test.

When I need series of values, I use concat(Stream<T>) → Promise<T[]> with similar approach: expect(await concat(s)).eq(sequence) (in async test).

@nordfjord
Copy link
Collaborator Author

@StreetStrider I do believe the test framework and eslint settings are due for an overhaul.

For instance the tests fail if I use arrow functions as that's not permissible by the es version.

With that in mind, how would you change the test?

@StreetStrider
Copy link
Contributor

@nordfjord with that in mind, my strategy still can be applied. Before async/await and arrows I'd just used function () { … } and return promise. mocha supports this, while async/await and arrows can be used as sugar.

@nordfjord
Copy link
Collaborator Author

@StreetStrider I've updated the PR returning a Promise instead of using done

@StreetStrider
Copy link
Contributor

Looks good.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants