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

Add documentation on how to test methods that return promises, linked… #2054

Merged
merged 1 commit into from
Jul 10, 2019
Merged

Add documentation on how to test methods that return promises, linked… #2054

merged 1 commit into from
Jul 10, 2019

Conversation

kobbikobb
Copy link
Contributor

@kobbikobb kobbikobb commented Jun 28, 2019

Issue #1898 is stale and talks about missing documentation for promises. Is this something we would like?

I can change the code to use dependency injection if we think more developers would use that. If we want that we need to decide constructor or property injection... :)

Purpose (TL;DR) - mandatory

How to verify - mandatory

  1. Check out this branch
  2. npm install

Checklist for author

  • npm run lint passes
  • References to standard library functions are cached.

@coveralls
Copy link

coveralls commented Jun 28, 2019

Pull Request Test Coverage Report for Build 2918

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 94.186%

Totals Coverage Status
Change from base Build 2911: 0.0%
Covered Lines: 1660
Relevant Lines: 1729

💛 - Coveralls

@fatso83
Copy link
Contributor

fatso83 commented Jun 28, 2019

Coincidentally, I looked into all of these issues yesterday and actually followed up on this! Weird timing :)

@orlangure already made a significant effort on this in #1931, which have unfortunately stalled for some time, but I think I'm merging this within a week. I'd rather have this build-upon that work, if that is fine with you? I'll ensure it doesn't drag on any longer than it needs to. Working beats perfect any day :)

So, if you could just set this aside for a week, I'd just copy-paste the additions into the (soon to be) existing article.

@fatso83
Copy link
Contributor

fatso83 commented Jun 28, 2019

OK, #1931 has been merged. Could you see if you have stuff that is uncovered in https://github.com/sinonjs/sinon/blob/master/docs/_howto/lolex-async-promises.md and copy it in?

@kobbikobb
Copy link
Contributor Author

@fatso83 my examples are realistic and show how to test simple async code with stubs and different arguments.

Lolex is intended for more complex scenarios, f.x. setTimeout, setInterval...

I think some developers might benefit in some opinionated and simple ways of testing async code.
The developers I work with thought you needed to call a done callback but that is not the case and they tend to not to test async code because they think it is too complicated.

I am open to anything if we want to move it, change it or combine this with other documentation.
You might think this is too general for our documentation and maybe it is.

What do you think? :D

@fatso83
Copy link
Contributor

fatso83 commented Jun 30, 2019

Right now, it's a demo of Mocha's API, along with some examples on doing async tests - which happen to use Promises. I think this is fine, but it's covering a bit of different ground than we were originally thinking of with #1898. I think your doc is basically the complement to your previous addition: https://github.com/sinonjs/sinon/blob/master/docs/_howto/stub-dependency.md.

Maybe it would fit better there, as a section on how to stub out dependencies that are async? It could have your existing stuff, but possibly also an example on doing the same for classic callback passing async code.

@kobbikobb
Copy link
Contributor Author

@fatso83 Good idea, I was thinking the same thing. I added them together and improved some texts.

The thing that is now bothering me is that the examples: simple and asynchronous are not working on the same codebase(one is more real like...). Maybe it does not matter.

What do you think, any feedback?

@fatso83 fatso83 merged commit 17ba30a into sinonjs:master Jul 10, 2019
@mroderick
Copy link
Member

I don't think the echo file was really necessary here ...

@mroderick mroderick mentioned this pull request Jul 10, 2019
franck-romano pushed a commit to franck-romano/sinon that referenced this pull request Oct 1, 2019
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.

4 participants