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

Argument Aliasing #687

Closed
wants to merge 1 commit into from
Closed

Conversation

jamestalmage
Copy link
Contributor

Aliases arguments so you can access them via meaningful property names instead of by array indexes.

Configure argument aliases, probably best used in setUp or beforeEach (setup is bit verbose for a single test).

var spy = sinon.spy();
spy.argNames = ["path", "callBack"];  //The first argument is called "path", the second "callBack"

Call the spy as you normally wood:

spy("a/b", fn1);
spy("b/c", fn2);

The spy object will now have custom-named properties, which are arrays populated with the named argument from each call. spyCall objects also get custom-named properties, they aren't (necessarily) arrays, but rather the actual named argument passed to that particular call.

assert.equals(spy._path, ["a/b", "b/c"]);  
assert.same(spy.firstCall._callBack, fn1);
spy.withArgs("b/c").firsCall._callBack(); //calls fn2

The "_" prefix is added to avoid collisions with existing spy properties.

There are at few real benefits to using this approach:

  1. Readability - firstCall._path conveys a bit more meaning than firstCall.args[0] (though perhaps it would be a bit clearer if I added the properties to the array instead - i.e. firstCall.args._path).
  2. Finding usages of a particular arg in your tests is easier. Grepping for ._path on your tests will likely return far fewer results than .args[0].
  3. Refactoring method signatures - If an argument changes position as your api evolves, simply fix the alias assignment in setUp, no need to refactor every test.

@@ -901,6 +901,72 @@ if (typeof require === "function" && typeof module === "object") {
}
},

".argNames": {
//jscs:disable disallowDanglingUnderscores
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I disable the "dangling underscore" lint rule for the just these tests. Otherwise this lint rule actually helps ensure that aliased arguments will never collide with existing api properties.

@cjohansen
Copy link
Contributor

Thanks for taking the time to contribute to Sinon. However, I'd advise to discuss new features before spending time implementing them. I'm not so sure about this, it seems to clutter things up a little. E.g. spy._path - what does it mean? The path argument from the last call? First? This is a kind of shorthand we don't have anywhere else in Sinon today. Also, I'm not enthusiastic about "magic" naming like adding underscores to the argument names, even if I appreciate the thought behind it.

If we were to support this, I'd prefer something like this:

var spy = sinon.spy(function (path, callback) {});
spy("a/b", fn1);
spy("b/c", fn2);

assert.equals(spy.getCall(0).args.path, ["a/b", "b/c"]);  
assert.same(spy.firstCall.args.callback, fn1);
spy.withArgs("b/c").firsCall.args.callback(); //calls fn2

It's not as concise, but there's also no magic, and properties are found where I'd expect them (e.g. arguments are accessed through args). But I'm not sure this is so helpful it's worth it - it feels a little like something that is mostly useful in overly complicated tests. @mantoni @mroderick thoughts?

@jamestalmage
Copy link
Contributor Author

It's not as concise, but there's also no magic, and properties are found where I'd expect them (e.g. arguments are accessed through args).

I agree and had that thought after I was already typing up the PR.

Also, I'm not enthusiastic about "magic" naming like adding underscores to the argument names, even if I appreciate the thought behind it.

Me either, but without it you run into naming collisions. Not a huge deal since sinon builds arrays using Array.prototype.push.call(...), etc. But we would have to think through any names we should not overwrite and test for them / throw (length is the only thing that comes to mind, but there may be others).

The idea of introspecting on the function to find arg names was one I already had, and I like it. But I didn't want to go down that road until I had feedback on the basic idea.

I'd advise to discuss new features before spending time implementing them.

The initial code / test really took no time at all, so I wasn't worried about that (and won't be crushed if this never gets merged). Had I known the rabbit hole your linter was going to send me down, I definitely would have done things differently. Lesson learned.

@mroderick
Copy link
Member

If we do decide to implement such a feature, I too would prefer to have regular names on the args object, than underscored names on the call object.

However, I cannot say that this is a feature that I've missed and I am not sure that it makes the resulting code that much clearer. If you're in a situation where you need this, you're probably better off simplifying your code...

@jamestalmage
Copy link
Contributor Author

closing due to lack of interest

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

Successfully merging this pull request may close these issues.

None yet

3 participants