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 dependency mocking documentation. #2010

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

theKashey
Copy link

@theKashey theKashey commented Apr 26, 2019

Purpose

Provide a better how-to, background and explanation for a dependency mocking. Make difference between different patterns a bit more clear.

Page changed: https://sinonjs.org/how-to/link-seams-commonjs/

PS: Not renaming the file to make a diff view possible.

Background

"Add documentation for how to stub ES6 module dependencies" - #1358 (comment)

Solution

I am not proposing a better solution, just providing a more rich explanation for the existing one.

@coveralls
Copy link

coveralls commented Apr 26, 2019

Pull Request Test Coverage Report for Build 2945

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 5 unchanged lines in 1 file lost coverage.
  • Overall coverage decreased (-0.005%) to 94.203%

Files with Coverage Reduction New Missed Lines %
sinon/behavior.js 5 96.06%
Totals Coverage Status
Change from base Build 2842: -0.005%
Covered Lines: 1666
Relevant Lines: 1735

💛 - Coveralls

Copy link
Contributor

@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.

Thanks for update! It's always nice with some more meat on the bone. There's some language cleanup that needs to be done, but that details. For the bigger picture, I think I'll hear what Morgan has to say.

/cc @mroderick


To better understand the example and get a good description of what seams are, we recommend that you read the [seams (all 3 web pages)][seams] excerpt from [Working Effectively with Legacy Code][legacy] before proceeding.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think removing this is improving the article. People read too little today 👴 That book is great and every seasoned dev should know about it.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry, but I shall disagree.

  • "Seams" are a quite rare thing, probably defined only at the mentioned article, and that a quite broad term.
  • while seams are "alter behaviour in your program without editing in that place", they are not about dependency mocking
  • provided examples are using or C preprocessor(not exists in JS), or Java class inheritance(might not exist in js). All examples are 100% alien to JS dev

I've read and wrote many articles related to this subject, and this one is probably most less useful.

Copy link
Author

Choose a reason for hiding this comment

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

About the articles. To say the truth - I don't know any good one, I truly want to recommend. I could share a few ones of mine, but they are also not great, and bound to rewiremock, which is a conflict of interest of some sort.

Copy link
Member

Choose a reason for hiding this comment

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

I stand by my recommendation of that book. Please drop this change.

Copy link
Author

Choose a reason for hiding this comment

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

Sure. But sooner or later it have to be replaced by a more js-related example, as long as the goal is to improve. Someone has to write it first.

The goal of the every article is to be useful for the reader, and provide some information their could use directly or indirectly, but just after reading. Unfortunately, patterns from the "3 pages" are not quite applicable to JS, or just don't exist in the same form:

  • page 2 seam - is portable to js class inheritance, almost as-is.
  • page 3 example 1(preprocessor seams) - there is no analog for preprocessor, except babel macros or require hooks, but that's not an option. The closest analog is using process.env.NODE_ENV==='test', but, again, that's not the same.
  • page 3 example 2(link seams) - seam at the linking stage is equal module dependency injection pattern.
  • page 3 example 3(object seams) - no analog, except inheritance seam from page 2, which is not the same.

So - if you have to idea how linking in C works, or what's the difference between C and JS class system(vast majority of devs) - then 0(zero) seams are applicable or understandable.

By fact there are 2 legit ways to use seams in js:

  • DI/providing callbacks into the constructor, so creating an object/calling a function, which shall be "pure", and call only provided callbacks in a response, not perform a real action. Especially not to do anything before "the call".
  • sandboxing - via class inheritance, module dependency replacement, and sinon.sandbox.
  • 😬 while devs are usually just overriding class methods or module variables "when it's too late", and that's what I am fighting with.

This page describes how to isolate your system under test, by stubbing out dependencies with [link seams][seams].
While in other languages you might use [link seams][seams], [Dependency Injection(DI)][di] or
[Inversion or Control(IoT)][IoT], which are not just _patterns_ but also requires some pattern
implementation(a framework) - CommonJS(aka nodejs) module system provides a way better to replace
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need a framework to do DI. I do it all the time by having optional constructor parameters. If using links seams really is "better" is debatable, as it introduces a new dependency, apis, complexity and mental overhead, but at least it doesn't require you to change the SUT.

In general, you don't need any custom framework to use IoC (of which DI is one pattern). Constructor injection or service locators are simple to implement in any language. Less mystery is one of our tenants, in any case, so we wouldn't want to give people the impression you need special tools, unless you are doing "special" stuff like hooking into the module loading system (as we are doing here).

Copy link
Author

Choose a reason for hiding this comment

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

  • DI. Accepting your dependencies via arguments, ie using some sort of a factory pattern(and able to produce multiple objects). That's more about runtime.
  • dependency mocking. Statically declaring your dependencies, without the ability to factor different versions. That's only about testing.

I would say - DI needs some framework, as long as you have to structure and design your code in a specific way, while dependency mocking operates on a bit different level and in a different time(tests only).

To be honest - I am quite happy that we have such ability in JS as a dependency mocking, so I don't have to implement 3 tier architecture to be able to mock fs. That makes code simpler and tests simpler.

There are more Java developers around me than JS, with all that Enterprise stuff from the fairy tales, and I like to show them how simple the code could be. (Sometime they do the same)

Copy link
Member

Choose a reason for hiding this comment

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

I agree with the opinion that dependency injection does not need a framework in JavaScript. I often use Sinon's spies, stubs and fakes in combination with pure functions. This makes my code easy enough to reason about, that I can get my colleagues to 👍 my pull requests.

docs/_howto/link-seams-commonjs.md Outdated Show resolved Hide resolved
docs/_howto/link-seams-commonjs.md Outdated Show resolved Hide resolved
fatso83 and others added 2 commits April 27, 2019 09:48
Co-Authored-By: theKashey <thekashey@gmail.com>
Co-Authored-By: theKashey <thekashey@gmail.com>
@stale
Copy link

stale bot commented Aug 7, 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.

@stale stale bot added the stale label Aug 7, 2019
@stale stale bot closed this Aug 14, 2019
@fatso83
Copy link
Contributor

fatso83 commented Aug 15, 2019

I'll try to let the good work put into this come to use by adding some additional commits on top of it to bring it more inline with our general thoughts.

@fatso83 fatso83 reopened this Aug 15, 2019
@stale stale bot removed the stale label Aug 15, 2019
@theKashey
Copy link
Author

Sorry guys. Every day I am trying to get some free time to finish this PR and every day there is less and less free time left.

@fatso83 fatso83 self-assigned this Aug 16, 2019
@stale
Copy link

stale bot commented Oct 15, 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.

@aiodh
Copy link

aiodh commented Feb 4, 2022

theKashey:master

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

Successfully merging this pull request may close these issues.

None yet

5 participants