Add throwOnNotFound method #225

jacobsmith-fusionalliance · 2018-10-29T12:12:50Z

Add an optional configuration function, similar in API to noCallThru,
that will force Proxyquire to throw an error if one of the stub keys is
not resolvable. Currently, it silently stubs out a module that is not
utilized by the object being proxyquired.

The intended use case is to aid in refactoring and debugging. Test
suites may optionally enable this feature so that developers get a
meaningful message when they attempt to proxyquire an import that is not
used, as, generally, this is a result of forgetting to update a
proxyquire statement after changing the name or imports of a module.

===

Thanks for all your work on proxyquire - myself and the team I am on have gotten some real use out of it! I have found that additional information when refactoring code would be helpful (i.e., we rename foobar.js => foobarFunction.js, forget to update our proxyquire({ 'foobar': ... }), and then the proxyquired object calls out to the actual foobarFunction, possibly having side effects or just generally making debugging a bit more difficult.

Please let me know if you want any changes to this PR (and no hard feelings if it's not something you want to bring in to the mainline (: ) - thanks!

Add an optional configuration function, similar in API to noCallThru, that will force Proxyquire to throw an error if one of the stub keys is not resolvable. Currently, it silently stubs out a module that is not utilized by the object being proxyquired. The intended use case is to aid in refactoring and debugging. Test suites may optionally enable this feature so that developers get a meaningful message when they attempt to proxyquire an import that is not used, as, generally, this is a result of forgetting to update a proxyquire statement after changing the name or imports of a module.

bendrucker

Hi! Open to this with:

A new name (~~imports~~)
Consideration for whether you should be able to enable this for a single stub a la @noCallThru

bendrucker · 2018-10-29T17:59:19Z

lib/proxyquire.js

@@ -140,6 +153,10 @@ Proxyquire.prototype._resolveModule = function (baseModule, pathToResolve) {
      paths: Module.globalPaths
    })
  } catch (err) {
+    if (this._throwOnUnresolvedImports) {
+      throw new ProxyquireError('Invalid stub: "' + pathToResolve + '" is not required by the proxyquired object')


Inclined to just re-throw here

…error handling

jacobsmith-fusionalliance · 2018-10-29T18:28:55Z

Great, thanks for those comments! I've updated the code with the new name and the error handling like you mentioned.

I'm not sure about adding this to a single stub - in my own uses, noCallThru is something we've implemented at the file level, so I don't have any personal experience adding this to a single stub. It seems like this particular addition would not be as useful on a per-stub basis because the ultimate goal is to make sure that the Subject Under Test really does require everything that you are proxying over, so in my mind it only makes sense at the higher "module" level.

bendrucker · 2018-10-29T19:04:58Z

lib/proxyquire.js

+ * @private
+ * @return {object} The proxyquire function to allow chaining
+ */
+Proxyquire.prototype.throwOnUnresolved = function () {


throwOnNotFound is the best I can come up with. Handling unresolved stubs is more of a description of how proxyquire works as opposed to what the user wants.

bendrucker · 2018-10-29T19:07:40Z

test/proxyquire-argumentvalidation.js

+
+  describe('when I pass a stub with a key that is not required on the proxyquired object', function () {
+    function act () {
+      const proxyquireThrowOnUnresolved = proxyquire.throwOnUnresolved()


Suggested change

const proxyquireThrowOnUnresolved = proxyquire.throwOnUnresolved()

proxyquire.throwOnUnresolved()

The return value is the main fn itself—assigning the return value implies a new copy of proxyquire which isn't the case here. This test mutates state it doesn't own and I'd rather that be apparent in case it causes future issues.

Ah yes, thanks for pointing that out! I've been using a generator function most of the time around proxyquire to have a "clean slate" for each test run so forgot it was actually mutating here!

bendrucker

Thanks! If you add docs to this to the readme I can merge and release.

jacobsmith-fusionalliance · 2018-10-30T13:36:48Z

Alright, the README has been updated - let me know if you'd like any changes! Thanks!

jacobsmith-fusionalliance · 2018-11-07T19:13:40Z

Hi @bendrucker - just wanted to follow up here. Thanks!

bendrucker

Left a comment on the docs. It's not sufficiently true to the behavior as written. I'm happy to knock out a grammar/clarity edit at the end but I want to make sure we agree on the substance.

Chain-ability with other methods (e.g. noCallThru) is implied but unimportant. What is important is that proxquire will only try to load your stub paths from disk when callThru is enabled. Set noCallThru and then throwOnNotFound does nothing. It's important to make this clear to users. This change does not validate all your stub paths if you don't enable call thru. Perhaps it should—that's still open for discussion as far as I'm concerned.

README.md

bendrucker · 2018-11-07T22:28:23Z

README.md

@@ -127,6 +127,41 @@ Two simple steps to override require in your tests:
    - therefore specify it exactly as in the require statement inside the tested file
    - values themselves are key/value pairs of functions/properties and the appropriate override

+## Raise error if proxying a modulePath that the module under test does not reference
+
+By default `proxyquire` will stub out modulePaths, whatever their path may be.


Do not line-wrap Markdown

bendrucker · 2018-11-07T22:31:09Z

README.md

@@ -127,6 +127,41 @@ Two simple steps to override require in your tests:
    - therefore specify it exactly as in the require statement inside the tested file
    - values themselves are key/value pairs of functions/properties and the appropriate override

+## Raise error if proxying a modulePath that the module under test does not reference
+
+By default `proxyquire` will stub out modulePaths, whatever their path may be.


Avoid "stub out," it's unclear what that means. It's also not true, as the sentence is written.

bendrucker · 2018-11-07T22:37:09Z

Sorry for the delay and thanks for keeping after it. Had some significant concerns with the docs and the behavior in general and just wrote them up.

samuelms1 · 2018-12-08T16:57:05Z

Just realized I opened a PR that does the same thing as this. See #228

samuelms1 · 2018-12-08T23:05:16Z

Actually - on second thought this PR is different from mine (#228). The feature I was trying to add was to force all dependencies to have a registered stub whereas this one seems to error if the path to a module was not found so the throwOnNotFound is appropriately named.

Co-Authored-By: jacobsmith-fusionalliance <43609909+jacobsmith-fusionalliance@users.noreply.github.com>

bendrucker reviewed Oct 29, 2018

View reviewed changes

Update throwOnUnresolvedImports to just throwOnUnresolved and update …

fcc9d1c

…error handling

bendrucker reviewed Oct 29, 2018

View reviewed changes

jacobsmith-fusionalliance added 2 commits October 29, 2018 15:16

update name to throwOnNotFound

a41e995

remove package-lock.json

a424158

jacobsmith-fusionalliance changed the title ~~Add throwOnUnresolvedImports method~~ Add throwOnNotFound method Oct 29, 2018

bendrucker reviewed Oct 29, 2018

View reviewed changes

jacobsmith-fusionalliance added 4 commits October 30, 2018 09:32

update Readme with information on throwOnNotFound

3a79f3d

update Readme with information on throwOnNotFound

f7d6ea4

bump for github

663d8ba

Merge branch 'master' of github.com:jacobsmith-fusionalliance/proxyquire

e75e3fe

bendrucker requested changes Nov 7, 2018

View reviewed changes

samuelms1 mentioned this pull request Dec 8, 2018

Added requireStubs() feature #228

Closed

Update README.md

4add017

Co-Authored-By: jacobsmith-fusionalliance <43609909+jacobsmith-fusionalliance@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add throwOnNotFound method #225

Add throwOnNotFound method #225

jacobsmith-fusionalliance commented Oct 29, 2018

bendrucker left a comment

bendrucker Oct 29, 2018

jacobsmith-fusionalliance commented Oct 29, 2018

bendrucker Oct 29, 2018

bendrucker Oct 29, 2018

jacobsmith-fusionalliance Oct 29, 2018

bendrucker left a comment

jacobsmith-fusionalliance commented Oct 30, 2018

jacobsmith-fusionalliance commented Nov 7, 2018

bendrucker left a comment

bendrucker Nov 7, 2018

bendrucker Nov 7, 2018

bendrucker commented Nov 7, 2018

samuelms1 commented Dec 8, 2018

samuelms1 commented Dec 8, 2018

	const proxyquireThrowOnUnresolved = proxyquire.throwOnUnresolved()
	proxyquire.throwOnUnresolved()

Add throwOnNotFound method #225

Are you sure you want to change the base?

Add throwOnNotFound method #225

Conversation

jacobsmith-fusionalliance commented Oct 29, 2018

bendrucker left a comment

Choose a reason for hiding this comment

bendrucker Oct 29, 2018

Choose a reason for hiding this comment

jacobsmith-fusionalliance commented Oct 29, 2018

bendrucker Oct 29, 2018

Choose a reason for hiding this comment

bendrucker Oct 29, 2018

Choose a reason for hiding this comment

jacobsmith-fusionalliance Oct 29, 2018

Choose a reason for hiding this comment

bendrucker left a comment

Choose a reason for hiding this comment

jacobsmith-fusionalliance commented Oct 30, 2018

jacobsmith-fusionalliance commented Nov 7, 2018

bendrucker left a comment

Choose a reason for hiding this comment

bendrucker Nov 7, 2018

Choose a reason for hiding this comment

bendrucker Nov 7, 2018

Choose a reason for hiding this comment

bendrucker commented Nov 7, 2018

samuelms1 commented Dec 8, 2018

samuelms1 commented Dec 8, 2018