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 requireStubs() feature #228

Closed
wants to merge 4 commits into from

Conversation

samuelms1
Copy link

  • Updated README with instructions
  • Bumped version to 2.2.0

- Updated README with instructions
- Bumped version to 2.2.0
@samuelms1
Copy link
Author

@bendrucker I just realized this PR is just like the already opened #225. You can close this if you like but you may like to compare them.

I've been wanting the feature that would force all modules to be registered for a long time (over a year). I should've made a PR back then. It's refreshing to see that someone else had the same idea and practically the same implementation, though it's interesting that our reasons for the feature differed. I think #225 was more about throwing if a module was not found because a path changed whereas I wanted to ensure that tests required explicitly all module dependencies and their associated stubs to prevent modules from unintentionally being loaded during a test - which by the way has bitten me many times!

Mocha for instance no longer terminates the process when finishing the last test so if you have some kind of unintentional background task running via setInterval or the like then the process will go on indefinitely until killed. I've seen logging libraries log to the console. I've seen proxies unintentionally called and in some cases in the background where it doesn't necessarily effect the test but unit tests shouldn't be making network calls. I like this feature for preventing unintentionally loading a module / making developers consciously choose (if they enable the requireStubs() feature) to pull in dependency modules as tests are written and modified.

I read through your comments in #225. I think I'll add a test to also ensure that stubs using @noCallThru: false continue to work as you'd expect. This could also be important behavior.

@samuelms1
Copy link
Author

I've compared with #225 a little further and found that the use cases are actually different. I thought they were the same initially, but #225 doesn't require that all modules be registered it just errors if a path to a module is not found rather than let things continue, so these are different.

- Modules loaded to support call through behavior are allowed to load dependent modules without registered stubs
- Updated README
@bendrucker
Copy link
Collaborator

Hmm. For something like avoiding network calls or spawning processes or intervals there are existing solutions: mocking net, child_process, setInterval, etc. globally.

I'm pretty skeptical that this is a good idea to use. Your implementation seems good, I appreciate the up front effort. If you can provide other examples I'd reconsider.

@dbunna
Copy link

dbunna commented Dec 11, 2018

+1 for this change.

@bendrucker This PR is something I've wanted to see out of proxyquire for a long time. When writing unit tests, I want to verify that I have accounted for all dependencies of the unit under test and not accidentally let one through so I can test in complete isolation. I'm confused as to why it wouldn't be considered as a useful and important change.

@bendrucker
Copy link
Collaborator

Hi, that doesn't make the case for the feature, it just restates what the feature does. I'm still wondering why you'd need to mock all dependencies. I'm not just looking out for your expressed need, but also what we're encouraging users to do.

I see this as an anti-pattern that encourages boilerplate and makes simple dependencies unnecessarily hard to use. On first glance I'd be disinclined to use it in projects I work on. I'm looking to be convinced otherwise before we'd entertain this feature as submitted. I'm especially interested in how this feature overlaps with overriding global behavior like net as a means of preventing tests from breaking out of their intended, isolated behavior.

@ronelliott
Copy link

@bendrucker to be fair, you're not stating why you feel it's an anti pattern. This feature helps teach newer programmers how to properly mock a module under test and ensure they have them all. Please don't hold this project hostage to your own personal opinions.

@bendrucker
Copy link
Collaborator

I am interested in #225 and will work on finishing it up soon. I am going to decline this and keep the normal require behavior for any un-stubbed modules. I understand some of the problems you referenced but there are other Node tools that already suit those needs, as mentioned above:

@bendrucker bendrucker closed this Apr 9, 2020
Repository owner locked and limited conversation to collaborators Apr 9, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants