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

De-duplicate the callback vs promise tests #149

Closed
victorb opened this issue Aug 25, 2017 · 4 comments
Closed

De-duplicate the callback vs promise tests #149

victorb opened this issue Aug 25, 2017 · 4 comments
Assignees

Comments

@victorb
Copy link
Contributor

victorb commented Aug 25, 2017

Currently, we duplicate all the tests (yay) so we're testing both the promise and callback implementations. While the idea is good, the implementation (copy-paste) is not ideal.

  1. Proposed implementation is to have one function with a callback that will automatically tests both implementations and remove all the duplicated tests.

  2. Another solution is to only test the callback vs promise interface in dedicated tests, and chose either callbacks or promises for the tests of the tests. But, we might miss some places by doing this.

@daviddias
Copy link
Contributor

We currently use Callbacks for all of the serious testing and then have a Promise test per function to check the promisify is there. I believe this is your proposed solution 2)

As for proposed solution 1), it does sound good. Wanna do a PR with that solution to see how it looks?

@victorb
Copy link
Contributor Author

victorb commented Aug 25, 2017

then have a Promise test per function [...] I believe this is your proposed solution

Proposal 2 would be more aggressive and just test callback/promise in one place, instead of per function. To ensure that the general idea is working. But, as mentioned, there is a risk of missing implementations that way.

Yeah, I'll give it a try and see how it works.

@victorb victorb self-assigned this Aug 25, 2017
@victorb
Copy link
Contributor Author

victorb commented Sep 7, 2017

The more I think about it, the less sense it makes to test callback/promise implementations all over the place. It would make more sense to abstract out the logic for taking care of the arguments in a separate tests and then the rest would be covered by promisify-es6 itself.

Only worry is where we are doing bunch of shuffling around of the arguments, which as mentioned should be abstracted away into it's own functions instead of being ad-hoc, but needs to be carefully done.

@alanshaw
Copy link
Contributor

Closed by #567 (only promises via async/await)

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

No branches or pull requests

3 participants