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

Improve docs for fakes #2070

Closed
itmayziii opened this issue Jul 27, 2019 · 18 comments · Fixed by #2360
Closed

Improve docs for fakes #2070

itmayziii opened this issue Jul 27, 2019 · 18 comments · Fixed by #2360

Comments

@itmayziii
Copy link

Is your feature request related to a problem? Please describe.
Reading through what a fake can and can not do is not clear from the documentation.

Describe the solution you'd like
Exact methods that are available to a Fake need to be documented.

First of all thank you for Sinon, I believe the library is powerful and has helped me write a lot of tests. What I'm requesting here is that Fakes need their documentation improved. Here is an example of something I struggled with.

The first sentence of the Fakes documentation says:

fake was introduced with Sinon with v5. It simplifies and merges concepts from spies and stubs.

The word "merges" here is too loosely defined to know what the API of a fake ends up being. My conclusion is that fakes seem to share an API with Spies with some additional stubbing behavior similar to stubs. I came to this conclusion because it seems that the fake object has the property called like a spy does (can't find this documented anywhere except in spies documentation), and withArgs that is usually available to stubs but does not seem available to fakes.

@fatso83
Copy link
Contributor

fatso83 commented Jul 28, 2019

Fakes definitely need better docs. Maybe something to address in the proposed (still undecided, @mroderick?) October hackathon?

@fatso83 fatso83 changed the title Documentation for Fakes seems lacking Improve docs for fakes Aug 7, 2019
@peterjgrainger
Copy link
Contributor

@fatso83 I would like to have a go at this.

Would I be updating the file https://github.com/sinonjs/sinon/blob/master/docs/release-source/release/fakes.md ?

Also is the scope of this issue to define which functions are available on a mock object, maybe linking to the spy and stub documentation?

@fatso83
Copy link
Contributor

fatso83 commented Oct 2, 2019

@peterjgrainger That's great! The scope and that approach seems reasonable ...

@peterjgrainger
Copy link
Contributor

peterjgrainger commented Oct 3, 2019

Looking at the code fakes seem to have the same API as a Spy and nothing from a Stub.

Propose updating the docs to remove the reference to stubs as it is a little misleading and adding that the API of fakes is the same as a spy

Shout 😮 if that doesn't look correct.

@mshaaban0
Copy link

@peterjgrainger Thanks for looking into this. I can see similarities from spy, and stub e.g. fakes.throws() is same as stub().throws()

@peterjgrainger
Copy link
Contributor

@mshaaban0 thanks for the input. I'll take a closer look

@peterjgrainger
Copy link
Contributor

peterjgrainger commented Oct 8, 2019

@fatso83 @mshaaban0 I think I misunderstood the scope.

I thought this ticket covered documentation of the fake function e.g.

var fake = sinon.fake();

not directly on the fake itself i.e.

sinon.fake

From what I can see sinon.fake() returns a spy I think this part of the documentation needs to be clarified as the reporter is asking for documentation on the object rather than the static functions.

I came to this conclusion because it seems that the fake object has the property called like a spy does (can't find this documented anywhere except in spies documentation)

@itmayziii seems to be referring to sinon.fake() rather than sinon.fake

I think I understand what to do now. Alter the current documentation to say which static functions are inspired by a spy and which are inspired by a stub and change the wording merged to better describe the mix of methods plus try to figure out how to distinguish between sinon.fake and sinon.fake()

@fatso83
Copy link
Contributor

fatso83 commented Oct 8, 2019

You're not the only one to misunderstand the scope. Glad you cleared it up for us 😄

@stale
Copy link

stale bot commented Dec 14, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@srknzl
Copy link
Contributor

srknzl commented Apr 17, 2021

I think I understand what to do now. Alter the current documentation to say which static functions are inspired by a spy and which are inspired by a stub and change the wording merged to better describe the mix of methods plus try to figure out how to distinguish between sinon.fake and sinon.fake()

What is needed to be done in this issue? If someone makes it clearer I would like to make a contribution.

Since there is a section called Instance properties, I think only methods of a fake function is needed to be explained. If there is no other methods on a fake function other than spy methods, I think there is no reason to refactor the current docs.

Thanks 😊

@fatso83
Copy link
Contributor

fatso83 commented Apr 20, 2021

@srknzl TBH, I do not remember all that well, but these are some points that could need improvement:

  • Make sure the list of static factory methods on sinon.fake is complete
  • Convert and/or add some runnable Runkit examples (very easy, but takes a bit of time)
  • Update the link in Instance properties to point to the same section in Spies.
  • State that the listed props are just one of the many from the spies documentation and that you can refer to that for the complete list
  • Clarify the wording so that it becomes clear that 1. The Fakes API is an alternative to the Stubs and Spies API that can fully replace all such uses. 2. It is intended to be simpler to use, maintain and has a lot smaller API surface and avoids many bugs by having quazi-immutable Fake instances (the behavior cannot be changed after creation (unlike Stubs), but they do have internal state that changes when used, like the callCount and other stats) 3. A Fake instance has the API of a Spy (is a Fake a Spy, @mroderick?)

@srknzl
Copy link
Contributor

srknzl commented Apr 21, 2021

The Fakes API is an alternative to the Stubs and Spies API that can fully replace all such uses

While this is true, I think using a spy instead of a fake + replace call is easier. Is there anything that can be done about this?

const foo = {bar: () => {}};
const aSpy = sinon.spy(foo, 'bar');
foo.bar();

console.log(aSpy.called); // true

vs

const foo = {bar: () => {}};
const barFake = sinon.fake(foo.bar);
sinon.replace(foo, 'bar', barFake);
foo.bar();

console.log(barFake.called); // true

If it was as easy as spies, fakes would be the only one that I'll use.

Edit:

I found out that I can do this:

const foo = {bar: () => {}};
sinon.replace(foo, 'bar', sinon.fake(foo.bar));
foo.bar();
console.log(foo.bar.called); // true

might worth to document this usage as well.

@fatso83
Copy link
Contributor

fatso83 commented Apr 22, 2021

@srknzl I am not sure if the replace call returns anything, but it could potentially return the fake. That would make it a bit smoother.

@fatso83
Copy link
Contributor

fatso83 commented Apr 22, 2021

Just verified:

f = sinon.fake();
val=sinon.replace(o, 'b', f)
assert.equal(f,val);

meaning you can write this:

const foo = {bar: () => {}};
const fake = sinon.replace(foo, 'bar', sinon.fake());
foo.bar();
console.log(fake.calledOnce); // true

@srknzl
Copy link
Contributor

srknzl commented Apr 22, 2021

yeap, makes sense. Also replace() does not state its return value on sandbox page

@srknzl
Copy link
Contributor

srknzl commented Apr 23, 2021

Just make it clear, which versions should change? All the versions that has fake, right? Or just the next version?

@srknzl
Copy link
Contributor

srknzl commented Apr 23, 2021

Another feedback about documentation in general:

I can understand that green ones are links, but some non-green ones are also links, like sinon.replace*, sinon.spy and sinon.stub. It would be better to somehow make them different
image

@srknzl srknzl mentioned this issue Apr 23, 2021
7 tasks
@srknzl
Copy link
Contributor

srknzl commented Apr 23, 2021

Initial pr is at #2360, work in progress.

I am waiting for your feedback.

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

Successfully merging a pull request may close this issue.

7 participants