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

Fake docs improvement #2360

Merged
merged 10 commits into from May 25, 2021
Merged

Fake docs improvement #2360

merged 10 commits into from May 25, 2021

Conversation

@srknzl
Copy link
Contributor

@srknzl srknzl commented Apr 23, 2021

Purpose (TL;DR)

Tries to improve documentation for the fake API. Resolves #2070

Todo

I went over this comment, that has these bullet points:

  • Make sure the list of static factory methods on sinon.fake is complete

yes, the list is complete except for usingPromise

  • Convert and/or add some runnable Runkit examples (very easy, but takes a bit of time)

I'm on it, I wanted open this pr so that you can check out current status.

  • Update the link in Instance properties to point to the same section in Spies.

Only firstArg was missing a reference, added that. Changed others' links so that they are linked to the exact same header.

  • 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 (

I've done my best to make it clear.

How to verify

  1. Check out this branch
  2. npm install
  3. cd docs
  4. gem install bundler
  5. bundle install
  6. bundle exec jekyll serve
  7. Check the website

Checklist for author

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

My Questions

Which versions should change? I changed v10.0.1 for now since I am not sure.

@srknzl srknzl mentioned this pull request Apr 23, 2021
### When to use fakes?

Fakes are alternatives to the Stubs and Spies, and they can fully replace all such use cases.

They are intended to be simpler to use, and avoids many bugs by having immutable behaviour.

The created `fake` `Function`, with or without behavior has the same API as a (`sinon.spy`)[spies].

Copy link
Contributor Author

@srknzl srknzl Apr 23, 2021

Choose a reason for hiding this comment

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

I thought adding this new lines to introduction would be too much for an introduction section. So moved one line from there to here.

#### Using fakes instead of spies

```js
var foo = {
bar: () => {
return 'baz';
}
};
// wrap existing method without changing its behaviour
var fake = sinon.replace(foo, 'bar', sinon.fake(foo.bar));
fake();
// baz
fake.callCount;
// 1
```

#### Using fakes instead of stubs

```js
var foo = {
bar: () => {
return 'baz';
}
};
// replace method with a fake one
var fake = sinon.replace(foo, 'bar', sinon.fake.returns('fake value'));
fake();
// fake value
fake.callCount;
// 1
```

Copy link
Contributor Author

@srknzl srknzl Apr 23, 2021

Choose a reason for hiding this comment

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

I think this alternative use cases helps as I stated in the issue #2070

@srknzl srknzl marked this pull request as ready for review Apr 23, 2021
@fatso83
Copy link
Contributor

@fatso83 fatso83 commented Apr 26, 2021

This is a great start, but the changes are in the wrong folder :) See https://github.com/sinonjs/sinon/blob/master/docs/CONTRIBUTING.md#documenting-the-next-release

@srknzl
Copy link
Contributor Author

@srknzl srknzl commented Apr 26, 2021

I moved the changes to latest docs. I wonder how I can run examples in the repo. I am asking because when I do the following I get an error:

  1. cd docs/release-source/release/examples
  2. npm i
  3. npm test
The error
Oops! Something went wrong! :(

ESLint: 6.8.0.

ESLint couldn't find the plugin "eslint-plugin-local-rules".

(The package "eslint-plugin-local-rules" was not found when loaded as a Node module from the directory "/home/serkan/forked/sinon/docs/release-source/release/examples".)

It's likely that the plugin isn't installed correctly. Try reinstalling by running the following:

    npm install eslint-plugin-local-rules@latest --save-dev

The plugin "eslint-plugin-local-rules" was referenced from the config file in ".eslintrc.yml".

If you still can't figure out the problem, please stop by https://gitter.im/eslint/eslint to chat with the team.

internal/modules/cjs/loader.js:960
  throw err;
  ^

Error: Cannot find module 'supports-color'
Require stack:
- /home/serkan/forked/sinon/lib/sinon/color.js
- /home/serkan/forked/sinon/lib/sinon/spy-formatters.js
- /home/serkan/forked/sinon/lib/sinon/proxy.js
- /home/serkan/forked/sinon/lib/sinon/stub.js
- /home/serkan/forked/sinon/lib/sinon/mock-expectation.js
- /home/serkan/forked/sinon/lib/sinon/mock.js
- /home/serkan/forked/sinon/lib/sinon/sandbox.js
- /home/serkan/forked/sinon/lib/sinon/create-sandbox.js
- /home/serkan/forked/sinon/lib/sinon.js
- /home/serkan/forked/sinon/docs/release-source/release/examples/spies-1-pubsub.test.js
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:957:15)
    at Function.Module._load (internal/modules/cjs/loader.js:840:27)
    at Module.require (internal/modules/cjs/loader.js:1019:19)
    at require (internal/modules/cjs/helpers.js:77:18)
    at Object.<anonymous> (/home/serkan/forked/sinon/lib/sinon/color.js:3:21)
    at Module._compile (internal/modules/cjs/loader.js:1133:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1153:10)
    at Module.load (internal/modules/cjs/loader.js:977:32)
    at Function.Module._load (internal/modules/cjs/loader.js:877:14)
    at Module.require (internal/modules/cjs/loader.js:1019:19) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [
    '/home/serkan/forked/sinon/lib/sinon/color.js',
    '/home/serkan/forked/sinon/lib/sinon/spy-formatters.js',
    '/home/serkan/forked/sinon/lib/sinon/proxy.js',
    '/home/serkan/forked/sinon/lib/sinon/stub.js',
    '/home/serkan/forked/sinon/lib/sinon/mock-expectation.js',
    '/home/serkan/forked/sinon/lib/sinon/mock.js',
    '/home/serkan/forked/sinon/lib/sinon/sandbox.js',
    '/home/serkan/forked/sinon/lib/sinon/create-sandbox.js',
    '/home/serkan/forked/sinon/lib/sinon.js',
    '/home/serkan/forked/sinon/docs/release-source/release/examples/spies-1-pubsub.test.js'
  ]
}

When I install npm install eslint-plugin-local-rules@latest --save-dev, and run npm test again another one occurs:

The other error
Error: Failed to load plugin 'local-rules' declared in '.eslintrc.yml': eslint-plugin-local-rules: Cannot find "eslint-local-rules.{,.cjs}(looking up from "/home/serkan/forked/sinon/docs/release-source/release/examples/node_modules/eslint-plugin-local-rules").
    at Object.<anonymous> (/home/serkan/forked/sinon/docs/release-source/release/examples/node_modules/eslint-plugin-local-rules/index.js:12:9)
    at Module._compile (/home/serkan/forked/sinon/docs/release-source/release/examples/node_modules/v8-compile-cache/v8-compile-cache.js:192:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1153:10)
    at Module.load (internal/modules/cjs/loader.js:977:32)
    at Function.Module._load (internal/modules/cjs/loader.js:877:14)
    at Module.require (internal/modules/cjs/loader.js:1019:19)
    at require (/home/serkan/forked/sinon/docs/release-source/release/examples/node_modules/v8-compile-cache/v8-compile-cache.js:159:20)
    at ConfigArrayFactory._loadPlugin (/home/serkan/forked/sinon/docs/release-source/release/examples/node_modules/eslint/lib/cli-engine/config-array-factory.js:978:42)
    at /home/serkan/forked/sinon/docs/release-source/release/examples/node_modules/eslint/lib/cli-engine/config-array-factory.js:848:33
    at Array.reduce (<anonymous>)
internal/modules/cjs/loader.js:960
  throw err;
  ^

Error: Cannot find module 'supports-color'
Require stack:
- /home/serkan/forked/sinon/lib/sinon/color.js
- /home/serkan/forked/sinon/lib/sinon/spy-formatters.js
- /home/serkan/forked/sinon/lib/sinon/proxy.js
- /home/serkan/forked/sinon/lib/sinon/stub.js
- /home/serkan/forked/sinon/lib/sinon/mock-expectation.js
- /home/serkan/forked/sinon/lib/sinon/mock.js
- /home/serkan/forked/sinon/lib/sinon/sandbox.js
- /home/serkan/forked/sinon/lib/sinon/create-sandbox.js
- /home/serkan/forked/sinon/lib/sinon.js
- /home/serkan/forked/sinon/docs/release-source/release/examples/spies-1-pubsub.test.js
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:957:15)
    at Function.Module._load (internal/modules/cjs/loader.js:840:27)
    at Module.require (internal/modules/cjs/loader.js:1019:19)
    at require (internal/modules/cjs/helpers.js:77:18)
    at Object.<anonymous> (/home/serkan/forked/sinon/lib/sinon/color.js:3:21)
    at Module._compile (internal/modules/cjs/loader.js:1133:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1153:10)
    at Module.load (internal/modules/cjs/loader.js:977:32)
    at Function.Module._load (internal/modules/cjs/loader.js:877:14)
    at Module.require (internal/modules/cjs/loader.js:1019:19) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [
    '/home/serkan/forked/sinon/lib/sinon/color.js',
    '/home/serkan/forked/sinon/lib/sinon/spy-formatters.js',
    '/home/serkan/forked/sinon/lib/sinon/proxy.js',
    '/home/serkan/forked/sinon/lib/sinon/stub.js',
    '/home/serkan/forked/sinon/lib/sinon/mock-expectation.js',
    '/home/serkan/forked/sinon/lib/sinon/mock.js',
    '/home/serkan/forked/sinon/lib/sinon/sandbox.js',
    '/home/serkan/forked/sinon/lib/sinon/create-sandbox.js',
    '/home/serkan/forked/sinon/lib/sinon.js',
    '/home/serkan/forked/sinon/docs/release-source/release/examples/spies-1-pubsub.test.js'
  ]
}

Installing supports-color does not help here, same error happens if I do that.

@fatso83
Copy link
Contributor

@fatso83 fatso83 commented Apr 26, 2021

@srknzl You touch upon something that I was fiddling with just a few weeks back, but I was surprised to see that I have not committed any of those changes. I don't remember quite what I was doing even ...

You can run the tests from the root easily:

npm run test-runnable-examples

That again only runs the same things you just did (from the examples folder), which runs the tests and does some linting:

docs/release-source/release/examples/run-test.sh

I did not get exactly the same errors as you did, but I think I fixed the issues up now. Try pulling and rebasing on top of master. In any case, the errors were just about the linting (no idea what the local rules thing was about), and the tests ran fine on my machine at least, even if eslint complained.

@srknzl
Copy link
Contributor Author

@srknzl srknzl commented May 2, 2021

I am trying to create fake examples with existing spy and stub examples. Is it a good approach? I can also add additional examples for methods that fake has but spy/stub does not have; what are they?

I finished spy examples. There are two interesting things:

  1. sinon.fake cannot wrap all the methods of an object, so I omitted that test.
  2. A fake instance does not have withArgs method, instead of it I used calledWith()

How many examples we will include for fake docs? Where we will put them? Any plans? I think it is better to plan this first.

Am I supposed to write runkit examples only for existing snippets in the fake page?

Copy link
Contributor

@fatso83 fatso83 left a comment

How many examples we will include for fake docs? Where we will put them? Any plans? I think it is better to plan this first.
I think the way you did it looks just fine! But AFAIK, they are just in the examples source directory and they would also need to be included on the page for them to show, so that means editing the markdown as done for the other types.

Am I supposed to write runkit examples only for existing snippets in the fake page?

Yes. Not sure what else to do? Sinon is a collaborative effort, so if you think that something is missing, then feel free to add the changes you would like to see.

docs/release-source/release/fakes.md Outdated Show resolved Hide resolved
docs/release-source/release/fakes.md Outdated Show resolved Hide resolved
@srknzl
Copy link
Contributor Author

@srknzl srknzl commented May 8, 2021

Converted all existing snippets to runkit + addressed your review points @fatso83 . I am open to another review at this point

@srknzl srknzl requested a review from fatso83 May 8, 2021
@fatso83 fatso83 merged commit 40f5ca3 into sinonjs:master May 25, 2021
@fatso83
Copy link
Contributor

@fatso83 fatso83 commented May 25, 2021

Thank you for all your efforts! Exemplary PR <3

@srknzl
Copy link
Contributor Author

@srknzl srknzl commented May 31, 2021

I realized I did not used runkit snippets I wrote. Why did not you warn me :) Should I fix it in a separate pr? @fatso83

@fatso83
Copy link
Contributor

@fatso83 fatso83 commented May 31, 2021

If you could, that would be great, @srknzl. Sorry, I did not check whether they were included in the markdown :(

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

Successfully merging this pull request may close these issues.

2 participants