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

Added Async/Await and Promise support and fixed a bug #3

Closed
wants to merge 1 commit into from
Closed

Added Async/Await and Promise support and fixed a bug #3

wants to merge 1 commit into from

Conversation

rpgeeganage
Copy link
Collaborator

@rpgeeganage rpgeeganage commented Jun 2, 2019

Hi @fatso83 ,
This is my first attempt to add async support to the mini-mocha.
In this PR,

  • we can use async/await and fixed the bug that beforeEach was not executed for all.
  • I have updated the demo file.
  • Please note that done callback is not supported. (Do you need that functionality?)
  • fixed the bug beforeEach does not execute for each test #1

Hope to get feedback from you.

Copy link
Owner

@fatso83 fatso83 left a comment

Choose a reason for hiding this comment

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

This is an outstanding piece of work. Thank you!

At first I was under the influence of the Not-Invented-Here syndrom when browsing the code and looking at the structural changes, but it really is much better than my initial attempt at this and a delightful display of how readable handling async code can be when using ES2015 syntax. I am not in love with the integration style testing using fixtures, but it works and is easy to reason about, so ✔️in my book. Pragmatism gets better results than dogmatism any day :)

I could just merge this as it is, as it is seemingly non-breaking and strictly adding features, but it doesn't address the most important aspect for me: handling the good old callback-style API. It's the most basic case, so for me, it IS a big thing to get right, but more importantly, I think you are 99% there. I added a suggestion for how you could handle it: basically wrapping any fn with length==1 in a promise before handing it over to the flow you already have going. That should work with it, before, afterEach and all the rest, although the actuals details may be a bit more involved than the simple addition mentioned above.

What do you think? Would you like to have a try at handling it in this one, which I think would make sense, or leave it for later? It would make sense to have it in this one, wrt the scope.

P.S. Could you rebase your changes into more meaningful commits - possibly just one?

index.js Show resolved Hide resolved
lib/execute.js Show resolved Hide resolved
lib/queue.js Show resolved Hide resolved
lib/util.js Show resolved Hide resolved
@rpgeeganage
Copy link
Collaborator Author

@fatso83 ,
Thank you very much for your input. I'll update the changes and we'll merge when I fix the callback functionality too. I'm working on it. 👍

@fatso83
Copy link
Owner

fatso83 commented Jun 3, 2019

The last changes seem great! Look forward to testing them out.

P.S. In case it's foreign, here's an intro to interactive rebasing.

Based on what I made on https://runkit.com/fatso83/sinon-test-issue-101

1.0.1

1.0.1

Handle multiple describe blocks

1.0.2

Update README.md

1.0.3

API change: setup() and explicit exports

Now with test coverage

Verify input

2.0.0

Gitignore

Fix typo for v2

2.0.1

Only include one file

initlai attempt

fix none async path

fix none async path

fix none async path

added async support

update demo file

restructure the code bit

small change

small change

added tests

Added callback support

Added callback support

update tests
@rpgeeganage rpgeeganage closed this Jun 3, 2019
@rpgeeganage
Copy link
Collaborator Author

I really suck at GIT stuff. sorry about this. ill open a new PR.

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.

None yet

2 participants