mocha
mocha copied to clipboard
mochajs
Context Variables and Functions
Here's a proposal for how we can add contextual variables and functions to make context blocks more useful and reduce redundancy when the same variable or function is needed within several nested contexts.
(Related to https://github.com/mochajs/mocha/issues/2656, but solving the same problem by exposing functionality that avoids the pitfalls of global usage)
In Ruby:
In Ruby's RSpec, it is a very common (recommended) pattern to reduce code redundancy by extracting before-each blocks that would be repeated in sub-contexts and setting relevant variables within each of the relevant contexts to account for any minor differences:
describe "getUsers" do
# a contrived example as this block doesn't constitute much redundant code, but...
before(:each) { create_list(userCount, :users) }
let(:userCount) { 1 }
it "returns the right user fields" { ... }
context "when there are 10 users" do
let(:userCount) { 10 }
it "returns a first page of 5 users" { ... }
end
end
The let function above is taking a block (function definition) and running it once per test, storing the result, and upon additional requests, returning the stored result (basically creating a singleton function out of the provided block) in order to:
- optimize for efficiency (avoid running the block more than necessary) and
- isolate the values from mutation by other contexts
The defined functions are available to all child contexts, but can be overridden by any child context, (as shown above) so that they can minimize redundancy in cases where the same value is valid for multiple contexts.
In Mocha:
Almost the same thing can easily be accomplished currently by assigning an arrow function to a property of thix.ctx within the scope of a context block, and then accessing that function with this.currentText.ctx within a beforeEach or it block. This works because ctx is "cloned" (technically child contexts are created using the parent's context as a prototype), passing all properties to the child, and allowing them to be overridden without affecting the parent context:
describe("getUsers", function() {
beforeEach(function() { factory.createList(this.currentTest.ctx.userCount(), 'user') });
this.ctx.userCount = () => 1;
it("returns the right user fields", function() { ... });
context("when there are 10 users", function() {
this.ctx.userCount = () => 10;
it("returns a first page of 5 users", function() { ... });
});
it("still has userCount 1 here", function() {
expect(this.currentTest.ctx.userCount()).to.equal(1);
});
});
The differences are that:
- It's more cumbersome, for lack of syntactic sugar
- It uses an internal object (
this.ctx) that it probably shouldn't be using without an exposed API - It's not converted to a singleton function
Proposal
I propose making a variable called something like scope available in the context block, which would allow the assigning of arrow functions, and in an actual test/before block, make a variable with the same name available, which has, as properties all the previously assigned functions as singleton functions, i.e.:
describe("getUsers", function() {
beforeEach(function() { factory.createList(scope.userCount(), 'user') });
scope.userCount = () => 1;
it("returns the right user fields", function() { ... });
context("when there are 10 users", function() {
scope.userCount = () => 10;
it("returns a first page of 5 users", function() { ... });
});
it("still has userCount 1 here", function() {
expect(scope.userCount()).to.equal(1);
});
});
Alternately, the scope available in a context block could be a function, so that you would set a scoped variable like this (which might make implementation easier?):
scope('userCount', () => 1);
Even better: scope could use defineProperty to define the property using an accessor descriptor on the context object, so that it doesn't have to be called using a function syntax!
AWESOME! :smiley:
Re. that last idea, keep in mind that typo'd function calls are an error but typo'd properties are just undefined, which may or may not be easy to discover depending on the usage case (and the last thing anyone needs is undetected incorrect behavior in a test, of all things; see https://github.com/chaijs/chai/issues/726 for example)
Re. the proposal itself, I know we discussed this in Gitter, but I am having a hard time recalling (and it'd be nice to get it documented here as well): is there a short explanation of what advantage this has over just using local variables/constants of the describe callbacks?
describe("getUsers", function() {
beforeEach(function() { factory.createList(userCount(), 'user') });
const userCount = () => 1;
it("returns the right user fields", function() { ... });
context("when there are 10 users", function() {
const userCount = () => 10;
it("returns a first page of 5 users", function() { ... });
});
it("still has userCount 1 here", function() {
expect(userCount()).to.equal(1);
});
});
Okay, touche :wink: So my example was not very good, hehe.
Obviously there's the syntactic sugar aspect, but that's not a huge difference, so here are the 2 cases where I don't think just using local variables would work:
describe("case 1", function() {
var role = () => "user";
var user = () => {
// insert complicated HTTP mocking stuff you don't want to repeat in each context, or whatever
createUser(role());
}
it("is not an admin", function() {
expect(user()).not.to.be.an.admin();
}
context("as an admin", function() {
var role = () => "admin";
it("is an admin", function() {
expect(user()).to.be.an.admin(); // I'm pretty sure this fails...
});
});
});
function passesSharedSpecs() {
it("does something every context should do", function() {
expect(this.testableThing().property).to.eq("x"); // this shared spec doesn't have access to locals, so we have to reference `this.something`;
});
}
describe("case 2", function() {
var thingOptions = () => {version: 1};
before(function() {
this.testableThing = () => createThing(thingOptions()); // This will not use the right thingOptions in sub-contexts
setupMockResponseFor(testableThing());
});
passesSharedSpecs();
context("in another context", function() {
var thingOptions = () => {version: 2};
// ^^ We can't use a before block to set 'this.thingOptions' either, because it would execute after the previous before block that uses it.
passesSharedSpecs();
});
});
I believe my proposed feature solves both of these problems.
Sorry, I could have been much more concise about what wouldn't work!
I my original example (and yours above @ScottFreeCode) we're doing:
expect(userCount()).to.equal(1);
When the case that actually breaks is:
// assuming we've assigned the result of the before block statement to this.users:
expect(this.users.count()).to.equal(1);
Because the before block that's supposed to create N users doesn't pick up on the local variable userCount
In general, most of these seem like they could be made to work with local variables with sufficient restructuring -- for instance:
describe("case 1", function() {
var makeUserFunction = role => {
// here do anything that you want set up once per role instead of once per call to user()
return () => createUser(role);
}
var user = makeUserFunction("user");
it("is not an admin", function() {
expect(user()).not.to.be.an.admin();
}
context("as an admin", function() {
var user = makeUserFunction("admin");
it("is an admin", function() {
expect(user()).to.be.an.admin();
});
});
});
function passesSharedSpecs(testableThing) {
it("does something every context should do", function() {
expect(testableThing().property).to.eq("x");
});
}
describe("case 2, no this", function() {
function makeTestableThing(thingOptions) {
var testableThing = () => createThing(thingOptions());
//setupMockResponseFor(testableThing()); I am ignoring this line for now because A) I am not sure what it's intended to do and B) if you call `testableThing()` for something used here and don't do the same in the nested context then whatever used it here will never get a version with a different `thingOptions` regardless of how `thingOptions` is changed
return testableThing;
});
passesSharedSpecs(makeTestableThing(() => {version: 1}));
describe("in another context", function() {
passesSharedSpecs(makeTestableThing(() => {version: 2}));
});
});
Refactoring would generally be our recommendation because it's almost always better for code to be more parameterizable and less dependent on state -- arguably the above higher-order functions are superior even to local variables, in that regard.
But assuming for sake of hypotheticals that you had code that:
- you can't significantly refactor
-
thiscan't reach the code that needs to be parameterized - local variables won't do because outer-scope code needs to respond to variable changes in inner scope
What if we tried to implement something like your scope variable proposal, but on the user side:
// scope.js
module.exports = function Scope() {
var scopes = [{}]
return {
current: function() { return scopes[0] },
push: function(addition) { scopes.unshift(Object.assign(Object.create(scopes[0]), addition)) },
pop: function() { scopes.shift() }
}
}
// test.js
var scope = require('./scope.js')()
function passesSharedSpecs() {
it("does something every context should do", function() {
expect(scope.current().testableThing().property).to.eq("x");
});
}
describe("case 2", function() {
before(function() {
scope.current().testableThing = () => createThing(scope.current().thingOptions());
scope.push({
thingOptions: () => {version: 1};
});
setupMockResponseFor(scope.current().testableThing()); // still dubious, setupMockResponseFor will never be called with testableThing with version 2, but even a built-in Mocha `scope` would not fix that
});
after(scope.pop.bind(scope));
passesSharedSpecs();
context("in another context", function() {
before(function() {
scope.push({
thingOptions: () => {version: 2},
});
});
after(scope.pop.bind(scope));
passesSharedSpecs();
});
});
Thoughts?
More generally, judicious use of a combination of outer variables with updates in before and after could create a similar effect with slightly more boilerplate -- e.g. having to save the previous context's copy of the variable in order to restore it in after. But if you want a more systematic, packaged solution this simple object is, if I've understood correct, very close in effect to what you're asking for (let me know if I'm wrong about that or have misunderstood), but without actually polluting the global namespace. And if you don't mind wrapping Mocha's callbacks in some way, you could conceivably replace push/pop with a single nest function that would call before and after with the relevant code, reducing the usage boilerplate even further.
Sure. I will agree that it can technically be done on the user side. The difference is that if I do it in a way that looks nice on the user side, I have to basically create my own DSL. Like, I would want to replace describe and context and before and it so that I don't have to wrap everything in a nest block. Because here's what the above test would look like with my proposed solution (about 1/3 fewer lines, excluding comments and spaces, and shorter property accessors):
function passesSharedSpecs() {
it("does something every context should do", function() {
expect(scope.testableThing().property).to.eq("x");
});
}
describe("case 2", function() {
scope.thingOptions = {version: 1}; // doesn't really need to be a function - implementation of `scope` could reset this for every test even without using defineProperty
before(function() {
// when this block is called for "in another context" below, `scope.thingOptions` would be version 2
// So `testableThing` would be version 2, and `setupMockResponse` would call the mock responder (e.g. sinon) to setup a mock for version 2
scope.testableThing = () => createThing(scope.thingOptions);
setupMockResponseFor(scope.testableThing());
});
passesSharedSpecs();
context("in another context", function() {
scope.thingOptions = {version: 2};
passesSharedSpecs();
});
});
Incidentally, I think your most recent confusion with setupMockResponse was because you moved the definition of thingOptions to within the before block. It's supposed to be outside the before block, so that it can be overridden by nested contexts.
And the confusion with setupMockResponse before that was actually the reason that solution wouldn't work (because there's no way to call testableThing in the function context you created and have it change as necessary).
Anyway, if we could figure out a way to get close to that pretty using user-side code, I'd believe an argument that this isn't a good addition to Mocha, but I don't see how that would be possible without overriding the DSL... which I guess would be okay... maybe??
The thing that makes me think it would be easiest to add it to Mocha is that right now I'm just using this.ctx and it's actually working great - just a little verbose, and I'm not sure it's safe :) (So it could benefit by a separation into its own safe namespace and some syntactic sugar)
I suppose scope may need to be a function(?), but my point is that I think most of these benefits could be accomplished by exposing and documenting a simple function from Mocha like:
// in context blocks:
function scopeAccessor() {return this.ctx.scope}
// in test blocks
function scopeAccessor() {return this.currentTest.ctx.scope}
//everywhere:
function scope(variableName, value) {
if typeof(value) == "undefined"
return scopeAccessor()[variableName] = value;
else
return scopeAccessor()[variableName];
}
(If that is a safe place to put the scope object... I'm not certain about that)
The issue I was pointing out with setupMockResponse was that it's only called in the outer before, therefore (unless I'm seriously misaken) it will only run once, therefore even if testableThing picked up the update to thingOptions you also weren't calling setupMockResponse again to update based on the new value of thingOptions. I'm not sure what setupMockResponse was supposed to do so I don't want to go too far out of my way guessing what the correct usage for it would be; but I am, on the other hand, fairly confident that whatever it should be can be adjusted in a manner similar to the adjustments I suggested to the other functions.
In any case the main points I wanted to make here are:
- Refactoring can get rid of the need for this behavior. (And the refactored versions may be the most short and elegant option, when it comes down to that.)
-
thisalready does it, it just isn't always easy in some cases to get its value into the function that actually needs it (I'm now regretting that I didn't put together an intermediate example surrounding ways the value that's passed tobeforeanditasthiscould itself be passed to other functions... but I don't want to get too sidetracked at this point) (also worth noting explicitly, although it seems from the examples that you are generally aware of it,this.ctxis the same object inside the suite callback instead of thebeforehook) - It's possible to implement an external alternative -- a local object that behaves closer to
thisinstead of to regular scope -- with only a little boilerplate. (I haven't necessarily presented the least-boilerplatey way to do it; even using my rough draft, you could.push({})and thenscope.current().variable = <new value>instead of passing{ variable: <new value> }, for example.)
Don't get me wrong, I can see how having a separate variable would be more versatile than this; but we need to be clear that this would be a mere convenience for what amounts to a special case. That doesn't automatically mean it's not a good candidate, but it has to be considered against both the cost of maintenance (it seems small if you're looking primarily at how few lines of code you can imagine doing it in; but imagine making sure that future changes to this don't break scope, or dealing with twice as many forms of test code when trying to address bugs or edge cases or to migrate user test code to a better method, or...), and against some intrinsic disadvantages (which I have alluded to but I should probably have laid out sooner):
- Additional pollution of the global namespace -- granted we've already got
itanddescribeglobally, but it's possible that somebody has code under test (not just test code, but application code the tests are checking) that avoidsdescribeetc. but happens to use a globalscopevariable. Sure, they shouldn't be doing that -- globals are bad practice (except, apparently, in test runners?) -- but that doesn't mean Mocha won't get blamed for breaking their bad code. (This is a somewhat minor issue since it could theoretically be an import, that would just be inconsistent with the default approach to the other test-related functions -- although I also have the vague impression that we'd like the import-based approach to getting the interface components to be default someday, I'd have to go look up past discussion on that. Basically this isn't insurmountable, but if we want to avoid trouble for our users then it makes the full solution less obvious than it would otherwise seem. The next issue is much bigger...) - We don't really want to encourage this kind of setup/cleanup technique (even using the existing
thisthat we're likely stuck with for backwards-compatibility reasons) in the first place -- only the refactoring solution (in combination with doing more setup/cleanup in the tests rather than in hooks) can help ensure tests are not depending on state from prior tests, and only the refactoring solution (in combination with doing more setup/cleanup in the test rather than in hooks) can give you control over the many nasty edge cases (and/or bugs) surrounding hooks (e.g. whether an outer suite'sbeforeEachshould run before every test of a nested suite or only once before the nested suite, what exact orderbeforeandbeforeEachin both outer and nested suites should run, what tests and hooks should still be run if a given test or hook fails/throws, and probably one or two other things I'm forgetting).
I should also add that we have had a couple similar proposals in the past and I vaguely recall that these sorts of counterobjections (it's doable using better alternatives, it's not the direction we want to promote) were provided by some of the more senior maintainers; given more time I might be able to dig them out of the issues history...
To be clear: we might very well end up doing something like this, to create a more workable alternative to this since people are already using that an some other kind of variable would be more flexible in some cases; but we're going to have to take these issues into account and weigh it against how much it's needed. But in the meantime (of which there is likely to be lots!) I want to make sure we can help you as much as possible without just waiting on such an update.
First of all thanks @ScottFreeCode! I really appreciate the consideration you've been putting into this! :smiley: Thanks for pointing out all of these issues. I will attempt to apply some refactoring and/or come back with a real world example that I can't work around nicely. I suppose it may just end up being a philosophical difference that I will need to get used to between rspec and mocha, where your latter points overrule any issue that can't be resolved as nicely using your former suggestions. A couple points though:
The setupMockResponse example is close to the pared-down/barebones outline of my real world use-case, but you are right - I failed to outline it correctly! That before block was supposed to be a beforeEach block (a typo between writing rspec and mocha tests - in rspec before means beforeEach) :confused: So this is what it's supposed to look like (and now I'm just using this.ctx where necessary here):
function passesSharedSpecs() {
it("does something every context should do", function() {
expect(mockApi.findRequest).to.include(this.testableThing().properties);
});
}
describe("case 2", function() {
this.ctx.thingOptions = {version: 1};
beforeEach(function() {
this.testableThing = () => createThing(this.currentTest.ctx.thingOptions);
mockApi.setupResponseFor('path', this.testableThing());
browser.pushSomeButtons();
});
passesSharedSpecs();
it("passes other spec", function() {
expect(mockApi.response).to.have_status(401);
});
context("in another context", function() {
this.ctx.thingOptions = {version: 2};
passesSharedSpecs();
it("elicits a successful response", function() {
expect(mockApi.response).to.have_status(200);
});
it("displays a message with the updated data", function() {
expect(page.messages).to.include(some_data); // (requires mockApi to be setup correctly)
});
});
});
So I can imagine how to refactor this, by just creating a named function out of the beforeEach block, and then calling it from beforeEach blocks within each context and passing in thingOptions as a parameter. That is a little more verbose (repeating the beforeEach block in each subcontext) and, in my opinion, harder to understand on first reading, but I could accept that as a workaround. Is there a more concise way you'd suggest?
To respond to your last two points quickly:
- Fair enough -
this.scopewould be sufficient, or an import :+1: - As long as there's a documented order, I don't think the order is too confusing. It's become second nature to me in RSpec that contexts' before blocks are run in the order in which they are defined and
before(:all)always precedes anybefore(:each)(which run once before each test no matter what level they are defined at). I don't think it's ideal to prevents users from utilizing the full power of nested contexts, to "save them from themselves" so to speak. Although I agree that it's not good to build tests that rely on interwovenbeforeandbeforeEachcalls. The real problem here regarding "cleanup" is using abeforeblock at all. All my tests use onlybeforeEach, in order to isolate the tests. If we were to implement my proposal in the ideal way (which would require a bit more code), it wouldn't require any cleanup, because it would provide a function that returns isolated values that are fresh for each test run. I believe it could actually encourage better behavior, because it would allow pairing a context with the variables that directly represent the context more easily, rather than re-implementing each context inside each nested context and/or using non-isolated local variables (var) which seem prevalent currently.
Mostly I agree that there are other ways to shrink redundancy even in these situations, when it becomes a real problem, but I am inclined to keep using this.ctx with arrow functions in the short term, because it seems like the cleanest and most understandable way to me (even without additional conveniences). But I'll give the refactoring a shot just to see for myself :smiley:
Oh, and while it is only a convenience - not a necessity - I don't think that it is only a convenience for a special case. Sure its usefulness is only obvious in special cases, but in RSpec, using let is a ubiquitous construct that many rubyists use by default for every spec involving contexts, because it is a more isolated way of defining variables than simply defining a locally scoped variable (which can potentially be altered by other tests in the same context), and also a more optimized way of defining variables that are used only in some sub-contexts, because where they aren't used, the function defining the variable is never run (as opposed to defining something in a beforeEach block which is only used in 50% of the covered specs). Both of these benefits could translate to Mocha if we were to implement it in the right way!
Well, that's a lot of good thoughts I'm going to have to chew over; thanks. 8^)
I took another look at that one example given the understanding that that one block was supposed to be beforeEach, and discovered a mistake I had been making about Mocha's design. I had been thinking that this in outer beforeEach/afterEach when running for nested suites' tests would receive the this object of the nested suite. After all, if they still received the this for their outer suite, then this wouldn't be any different from local variables in the suites -- shared by all runs within said suite's tests and hooks, and same for nested suites that don't shadow the variable -- only harder to access because you need this to get to it. There's no point in such a construct, a crippled version of a suite-callback-local variable, right?
Anyhoo, I was thinking (based on that understanding) that the second example, if the outer hook was beforeEach instead of before, should work just fine if the vars were changed to setting this properties inside before blocks (which are only run once per suite), since I'm pretty sure nested suites' before blocks should run before outer beforeEach blocks (if they didn't it would be inconsistent, since except for the first test all the others have to have the outer beforeEach run after the nested before since the nested before has to run at some point before the first test and is not run again). I rigged up a this-property-setting and console-logging bundle of tests and hooks in nested suites purely to confirm that there aren't any bugs preventing those hooks from being run in the order I think they ought to be...
...And found out that they are run in the right order, but the outer Each hooks get the outer this property values, like a crippled suite-callback-local variable.
I have literally no clue what good this design was ever supposed to do, and I am now wondering how many issue reports we've gotten claiming that hooks run in the wrong order were misattributing to order what is fundamentally the uselessness of this instead.
:sigh: So, where does that leave us... Well, for starters, I think I've answered my question, "How is the proposal different from the existing this behavior?" since it turns out the existing this behavior not only isn't what I thought it was, but is basically useless...
And I'm going to have to chew over the rest till I understand exactly what's being asked for, in order to start making recommendations about how best to get it -- whether through a Mocha update or through some userland pattern or construct.
Indeed, not only is this useless inasmuch as it's just a broken version of a suite-local variable (EDITTED TO ADD: actually, this is only true of hooks, see next comment for rediscovery), but it just hit me that judicious use of local variables actually can achieve what I had erroneously thought this was needed for (but that this actually cannot do ~at all~ in hooks), with (depending on circumstance) the requirement that you explicitly reset the value even if no other cleanup would have been required:
function passesSharedSpecs() {
it("does something every context should do", function() {
expect(this.testableThing().property).to.eq("x"); // this shared spec doesn't have access to locals, so we have to reference `this.something`;
});
}
describe("case 2", function() {
var thingOptions = () => ({version: 1}); // also corrected to return object instead of running block containing nothing but a labelled `1` statement (not sure why JS allows labels before statements other than blocks and loops, you'd think there'd be no opportunity for `1` to use the label...)
beforeEach(function() { // corrected to Each
this.testableThing = () => createThing(thingOptions());
setupMockResponseFor(testableThing());
});
passesSharedSpecs();
describe("in another context", function() {
var outerReset = {};
before(function() { // NB: runs just once before anything else in this suite, including outer Each hooks
outerReset.thingOptions = thingOptions;
thingOptions = () => ({version: 2}); // same arrow function syntax correction as above, same rant against JS allowing useless label
});
after(function() { // NB: runs just once after everything else in this suite
thingOptions = outerReset.thingOptions;
});
passesSharedSpecs();
});
});
...And you only need after to reset it if you've got another nested suite that will run after the first one but should get the outer suite's instance of the variable again; you can just use this as the nested suite if you're confident it will be the only suite on its level or that all other suites on this level will also set the variable:
describe("in another context", function() {
before(function() { // NB: runs just once before anything else in this suite, including outer Each hooks
thingOptions = () => ({version: 2}); // same arrow function syntax correction as above, same rant against JS allowing useless label
});
passesSharedSpecs();
});
Obviously, although this works, it is a little more imperative (at least where the variable needs to be reset afterward) and a little riskier, not to mention the theoretical bad-idea-ness of carrying around that state...
Here's a quick question: If what you want, ultimately, is for functions to provide on-demand (well, not doing the work unless they're used) some resources that are initialized new for each test rather than carrying over from test to test, then do you need beforeEach-type hooks in the first place? Or would something like this work? Because it occurred to me that this (and .ctx, which as far as I can tell is the corresponding this for tests and hooks run in that suite; I think that also means that this.currentTest.ctx === this should be always be true in hooks) would see the more-nested stuff if called from the more nested stuff instead of from less-nested hooks (in which case it's less useless than it initially seemed, the hooks just aren't so involved in the good way to use it).
function passesSharedSpecs() {
it("does something every context should do", function() {
expect(this.testableThing().property).to.eq("x"); // this shared spec doesn't have access to locals, so we have to reference `this.something`;
});
}
describe("case 2", function() {
this.ctx.thingOptions = () => ({version: 1}); // also corrected to return object instead of running block containing nothing but a labelled `1` statement (not sure why JS allows labels before statements other than blocks and loops, you'd think there'd be no opportunity for `1` to use the label...)
this.ctx.testableThing = function() { // note use of regular function to be able to access the `this` parameter where the function is called from
var thing = createThing(this.thingOptions());
setupMockResponseFor(thing);
return thing;
};
passesSharedSpecs();
describe("in another context", function() {
this.ctx.thingOptions = () => ({version: 2}); // same arrow function syntax correction as above, same rant against JS allowing useless label
passesSharedSpecs();
});
});
And then, if you really want to avoid the ctx property and stick to documented stuff, you could switch this.ctx to just this and move the assignments of testableThing and thingOptions into before hooks (if you don't want to be recreating the functions themselves with every test) or into beforeEach hooks (if you don't mind recreating the functions themselves with every test); the important part is, they do get access to the current test's this as long as that access isn't happening until called from the current test. (Thus, before/beforeEach might be good tools for controlling exactly when some dependencies get reset, but for visibility of most-overridden function versions you're going to need to ensure that this.<whatever may be overridden> is called during the test itself.)
I obviously don't use this sort of design pattern often enough in the first place that it took me three tries to get it right, first discovering that my initial idea was all wrong and then having to rediscover that my next thought that the thing is useless wasn't quite right either...
Version with the more complete example:
function passesSharedSpecs() {
it("does something every context should do", function() {
expect(mockApi.findRequest).to.include(this.testableThing().properties);
});
}
describe("case 2", function() {
this.ctx.thingOptions = {version: 1};
this.ctx.testableThing = function() { // needs to be regular function to get `this`
var thing = createThing(this.thingOptions);
mockApi.setupResponseFor('path', thing);
browser.pushSomeButtons();
return thing
};
passesSharedSpecs();
it("passes other spec", function() {
expect(mockApi.response).to.have_status(401);
});
describe("in another context", function() {
this.ctx.thingOptions = {version: 2};
passesSharedSpecs();
it("elicits a successful response", function() {
expect(mockApi.response).to.have_status(200);
});
it("displays a message with the updated data", function() {
expect(page.messages).to.include(some_data); // (requires mockApi to be setup correctly)
});
});
});
Also, congratulations, you've inspired a new construct in the homegrown smarter-setup-and-cleanup-management test runner I'm experimenting with. 8^) :+1:
So, my personal opinions on how best to do this, on the assumption that "actually call the functions from the test that needs them so they can use this as the current context" is an acceptable solution for the most part*:
- I like the fact that with
ctxyou can directly attach these functions, no hooks needed. - I wouldn't worry about
ctxchanging. It's not declared private by_namingConvention; besides, even if it were, it wouldn't change without a major version bump considering the high probability of someone out there depending on it anyway. And in practice there's no reason to change it unless the underlying logic gets a major overhaul -- and historically we haven't been big on spending our limited time on major overhauls (or maybe we just don't have enough time for it to make a difference?)... - It might be nice to improve things so that people can use this technique with functions and have a harder time "getting it wrong" either with mutable stateful variables or with all the similar approaches we've tried here that don't quite work. However, I don't see any way to get rid of most of the ways it can be "gotten wrong" without messing up a lot of existing code. So I'm not too worried about adding a new safer way to do things when we'd still be leaving all the unsafe ones there; only in making sure the "good way" is as obvious as possible.
- The thing I don't like about
ctxis it's inobvious/unsemantic/unexplicit -- what is a "see tee ex" and why would you attach functions to it? I would probably want to check with the rest of the team, but would be inclined to accept a PR creating and documenting an alias with a name at least as clear asvalueOf_this_InHooksAndTestsbut less clumsy, wherein said documentation would outline this use case (and correct usage for it) in particular.
What do you think? (I am hoping I've finally got my head around the issue and the proposal enough that this idea's actually helpful, heh heh.)
*I should mention that, due simply to using functions, there's room for optimization with this strategy too: since function results are sort of like lazy values (so you're already getting the "it isn't setup/computed unless the test uses it" optimization) and can be memoized (I can elaborate, but basically, there are ways that one of these resource-providing context-dependent functions could reuse existing objects on subsequent calls after they're first computed for a given context or set of inputs to creating said object if such reuse is correct and more efficient).
Oh wow!
this.currentTest.ctx === thisshould be always betruein hooks
Well, okay, so I don't think that's actually true, but it IS true that this.currentTest.ctx in a hook == this in the test, which I didn't know, and which is handy! (and maybe that's what you meant?) :smile: But like you mentioned (I think??) while the hook can set properties of this which are then associated to this in the test, it cannot read this.currentTest.ctx.property using just this.property (i.e. they are not actually the same object, they just get merged after the block, somehow, apparently)
Anyway, wow, yes, you totally have it figured out! :) I think I agree about your conclusions too. Automatic memoization would be a nice feature to have, but a great first step would be, like you said, just exposing a name that makes sense and documenting how to use it! :+1: (I do wonder if rather than linking directly to this.ctx though... maybe it should link to a new property of ctx like this.ctx.scope to avoid mingling the new scoped variables with other members of ctx, although that does make it so you can't just use this inside the test... not sure which is more important)
The one thing I don't like about that last example (just to further prove that aliasing ctx is a good idea), and about the whole "actually call the functions from the test that needs them so they can use this as the current context" idea is that if you look at that example, it wouldn't be setting up the mock API response in time. The beforeEach block was there to ensure the mock API is ready to accept requests (and that the right buttons get pushed for the test) without having to call a function to setup that response and push those buttons at the beginning of each test (where it would need to be in order for the expectation to pass). Of course, it wouldn't be the worst thing to call one function at the beginning of a test, but I would definitely want to avoid the pattern of setting up an accessor that has side-effects that implement the actual context, and rather create a function called "setupContext" or something... but beforeEach is certainly still the best option for that case if we can expose/alias this.ctx :smiley:
Thanks for the further investigation! So... should I try to come up with an alias? I'd have to think a bit to imagine what would be both simple and extendable for the future. Do you like this.scope? or it could be a couple of functions like this.set and this.get? (which would allow for additional functionality in the future like automatic memoization and lazy evaluation...) I also like the idea of an import that could just be called scope... (although that might be hard/impossible to implement in a way that it would work both inside and outside of hooks/tests)
FWIW: The instinct I originally followed was to define my own simple alias like this in the global suite:
beforeEach(function() {
this.context = this.currentTest.ctx;
});
...so that I could use this.context within hooks & tests, which made it just palatable enough (except that it still felt dirty that I was stuffing all my variables into an undocumented ctx object I still don't quite understand) :wink:
Good catch about currentTest.ctx -- that should be the this of the nested test, the very thing I expected the hook's this to be but it isn't. I think that the ctx variable of test objects and suite objects (which are currentTest and this in hooks and suite-callbacks respectively) are the same object passed to the hooks and tests as this (except that the one corresponding to the suite is what's passed to them, so this in outer Each hooks isn't this in the corresponding test). Which, if I'm right, would mean that in effect hooks can get either their suites' tests' this or the current tests' this, no limitations or problems, the latter is just... again, ugly.
The rest of this comment is written under the assumption that that's the case and would have to be revised if it's not; I haven't thoroughly tested it, but this outlines the stuff we'd want to try to test in light of what I'd like to try to accomplish. I'm going to run over a lot of stuff again, but hopefully this time all of it will be on point.
So, speaking of ugly, it occurs to me that another value in an alias (or accessor method...) would be to unify these three ways of getting to the most-nested context:
-
this.ctxin suite callbacks -
this.currentTest.ctxin hooks -
thisin tests
So what if...
- the main object/function is
this.<alias or accessor>in tests - in suite callbacks,
this.<alias or accessor>accessedthis.ctx.<alias or accessor> - in hooks,
this.<alias or accessor>accessedthis.currentTest.ctx.<alias or accessor>
(Or, in other words, this.<alias or accessor> in things other than tests is a reference to the tests' this.<alias or accessor>.)
Tricky bit: If we didn't define this.<alias or accessor> for hooks, they'd already have it -- but it would be that suites' tests' this.<alias or accessor>, not the current test's. So Mocha would have to add a hook-specific this.<alias or accessor> when it adds currentTest and revert it when it removes currentTest.
Still... it seems like it should work.
Memoization is a bit tricky, on reflection. Normally, you memoize on the arguments to the function. But in this case we want to memoize at least in part on the properties of this (or the alias/scope-accessor/whatever instead) that are used by the function to be memoized. I think the safest way to do this is to create a memoization function that takes both an array of scoped properties/functions to look up and a function to memoize passing them as its arguments, also denying the memoized portion access to this that might circumvent the memoization. So it'd look something like this:
function memoizedResource(thisDependencies, once) {
// use memoize from some other library
once = memoize(once)
return function wrapped() {
// arrow functions still get access to `wrapped`'s `this`
return once.apply(null, thisDependencies.map(entry => this.<alias>[entry]()))
}
}
...
this.<alias>.thingOption = () => ({version: 1})
this.<alias>.testableThing = memoizedResource(["thingOption"], function(thingOption) {
return createThing(thingOption)
})
beforeEach(function() {
mockAPI.whatever(this.<alias>.testableThing())
}
Or, if built-in, like this:
this.<accessor>("thingOption", [], () => ({version: 1}))
this.<accessor>("testableThing", ["thingOption"], function(thingOption) {
return createThing(thingOption)
})
beforeEach(function() {
mockAPI.whatever(this.<accessor>("testableThing")<()?>)
}
Weird how that resembles AMD modules and dependency injection, but hey, it happened naturally, right?
(I haven't thought in-depth about how to implement the built-in version -- the array lookup has to grab stuff out of the same scoped store instead of this.<alias> directly, whatever that would end up involving, but it should be possible. Might be as simple as changing this.<alias>[entry]() to this.<accessor>(entry)<()?> in the implementation? Hopefully...)
Anyway, should it be built-in? That would mean people get it who wouldn't have thought to implement it themselves. It would also mean that people have to understand they can't, say, use functions with side effects or that are expected to reset state like a beforeEach hook inside the scoped function. It would mean that it's up to Mocha to pick the optimal implementation of the memoization proper. It would also mean that in pathological cases it could add more overhead than it saves, but I'm pretty sure that would not be normal. My bias, of course, is to avoid work on our end and promote separation of concerns and customization; but in this case I'm on the fence.
Do we need a form that would not be memoized, or could you always access scoped-to-current-test data through this accessor in order to use it in performing non-memoized state resets in beforeEach hooks? For instance, assuming the mock API and browser need to have their state reset before every test to be safe regardless (that is, that one or both of them shouldn't be memoized like testableThing is), would we ever need to ensure that the mock API or browser stuff is called based on testableThing being accessed rather than counting on there being an appropriate beforeEach hook handling them?
I like scope or scoped as an alias and scoped as the name of an accessor. It's not necessarily perfect, but it seems like it fits ok and I'm having a hard time thinking of something better.
Final consideration: is it important to get the friendly name and/or built-in memoization if Mocha might change the this behavior to be the current most nested one in the future? On the face of it, such a change seems unlikely; however, I don't know if it would be out of the realm of possibility if, say, it turns out that that's the way all other JS test runners with beforeEach/etc. and this work (so we'd be moving into alignment with them).
This may be a moot point depending on research into other test runners, which I need to go do when I've got a little time.
Any thoughts, concerns or suggestions at this point? If not, I think I'd like to write up a succinct rationale and design to summarize the various considerations and conclusions we've reached, so I can have the team review our final proposal in depth without having to read through my rambling chasing down red herrings in this thread.
Great ideas :+1: So I'll call it scope (and/or scoped) below, for brevity, but I agree it isn't perfect:
Alias:
Yeah! I like the idea of using the same name (i.e. this.scope) in all contexts to access the current tests's "scoped variables". I can't think of a use case where you'd want or need to access the current suite's non-overridden version of an overridden variable inside a hook, other than resetting, but any resetting should be to a known static state, not to some stored "previous state". That's what we're trying to fix :smile:
Memoization:
Good points... in RSpec's let your memoized function block is not allowed to have any parameters, so it's just memoized by name, but to balance that, the block itself does have access to self so that when it is first run, it can use other let-defined variables/methods (technically they are all methods). Would that be possible here?? At least the first part would be - when calling a function using scope(d), the memoizing function could simply call the given function with no arguments, enforcing the no parameter requirement. But as far as giving it access to this, you mentioned that that would allow it to circumvent the memoization... I'm not sure I quite get it, I have to think about it more.
Actually, perhaps memoization isn't exactly the right word for what we want. Or at least, the concept feels slightly different. Because all we really need is that in each test, the method is only called once, and that result is returned for every subsequent call until the end of the test - even if the context has changed, or the value itself has been modified later in the test. And at the end of each test, all the results should be cleared for the next test. So my thought was that calling this.scoped('testableThing', scope => { return createThing(scope.thingOption) }) would define scope.testableThing as some wrapped function that checks for and returns this.scopedResults.testableThing and if that's null, calls the original function, passing in this.scope and then stores the result and returns it... like:
function scoped(name, func) {
if typeof(this.scopedResults[name]) == "undefined" {
this.scopedResults[name] = func(this.scope);
}
return this.scopedResults[name];
}
Then, of course, there would have to be a global after hook that simply destroys this.scopedResults (or I suppose maybe the entire context of the current test's this is already destroyed at the end of the current test... which is perfect) :smile:
I can't think of a use case for a non-memoized version... I think the potential use case you described is maybe alleviated (although I didn't fully understand it) by resetting all the stored results after each test?
If this changes:
Umm... if it were the case that this were the same in before and beforeEach as in tests, I think technically we could use before blocks to define these overridable variables/functions with a fairly elegant syntax, with the caveat that we wouldn't get a dedicated namespace (since presumably, this would still contain some other necessary properties). The benefit of an alias/accessor would still be there I think (eschewing before blocks, dedicated namespace, possibly some form of memoization), but it certainly wouldn't be as dramatic ... enough that I probably never would have brought it up if this were the case :wink: But that does sound like a pretty significant change that could foil certain other use cases, potentially. Nevertheless, I think it would solve this problem!
Good points... in RSpec's let your memoized function block is not allowed to have any parameters, so it's just memoized by name, but to balance that, the block itself does have access to self so that when it is first run, it can use other let-defined variables/methods (technically they are all methods). Would that be possible here??
This is basically what I was getting at. Traditional memoization reuses outputs based on the input parameters. We don't want parameters calling from the outside, we want other scoped values.
(Basically -- although your further clarifications indicate this isn't quite what RSpec does -- I was thinking of not rerunning the function except when the other scoped values it depends upon are different, hence all the fiddling to get it to be memoized on whatever it needs to pull out of the scope.)
But as far as giving it access to this, you mentioned that that would allow it to circumvent the memoization... I'm not sure I quite get it, I have to think about it more.
I probably should have said "break" rather than "circumvent" -- if your function returns the same result whenever the arguments are the same values but it's allowed to access arbitrary values on this and they don't affect (niether one way nor the other) whether its results are reused, then the this access wouldn't work as expected: when the properties on this are different the function still won't be rerun, and you'll be stuck with the result from whichever this.property values happened to be in place the first time it happened to run with that set of arguments. (In the case of my design, in order to get memoization on other scoped properties working, the scoped properties were first converted into arguments, so the point here was disabling this access in addition in order to complete the transformation, I guess. Although I suppose there are other reasons for our scoped functions to not have access to this in the new design?)
Of course, that could also have been resolved by including this as part of the parameters to be memoized upon, but then the memoization doesn't reuse the function in different scopes (suites) even if the other scoped properties it's using are all the same -- and I was thinking of memoizing in the sense that multiple tests would reuse the same scoped resource (so not reusing it between suites would be an arbitrary pessimization in that design).
That being said...
Because all we really need is that in each test, the method is only called once, and that result is returned for every subsequent call until the end of the test ... And at the end of each test, all the results should be cleared for the next test.
You've anticipated what I've been pondering for the past couple days. 8^) While we can tell people they can't use it for resources that need to be reset with every test, I have been wondering whether that isn't something that's risky anyway in the same way that before is risky (the before-all version, not beforeEach), due to carrying state over between tests.
I hadn't thought of it before, but such pseudo-memoization as you suggest, something akin to memoization but based on the even more limited scope/lifetime not of suites but of tests, would be a good compromise: you don't accidentally recreate the thing many times if you use it in multiple places in a test, but you don't accidentally carry it over between tests either.
And, come to think of it, that's the hardest form of something-like-memoization to implement without Mocha's help -- I am not sure how userland code would tell when the current test has changed (except beforeEach/afterEach, but I'm not sure how easily those can affect control of scoped stuff from userland). So the benefit of having it built-in also goes up a lot when this particular substitute for memoization is the desired behavior.
Yeah, I really like this idea the more I think about it: don't actually memoize, but save the results and clear them on test completion.
I can't think of a use case for a non-memoized version... I think the potential use case you described is maybe alleviated (although I didn't fully understand it) by resetting all the stored results after each test?
Yup, I was thinking of memoization as in the same value is retrieved by multiple tests but considering the problem of some things needing to happen again for every test that depend on that value; "memoization" as in the same value is retrieved by multiple calls during the same test but it's wiped between tests has no such problem because it would happen again the first time for every test.
So, sounds like we're good to go with that idea; not sure exactly how complex the implementation's going to be (we'll want to store the values somewhere that the test and hook callbacks can't access except through scoped but that the runner can wipe once it's done with a given test's afterEach hooks), but while we work that out I guess I can go do that research on other test libraries and this in outer (before|after)Each that I want to know for comparison.
Perfect! Sounds great to me! :+1:
I like the idea of storing the cached results out of reach as well if it's not too complicated, but even storing them somewhere in this wouldn't be that bad if it's easier (particularly if that context already disappears at the end of a test, which is the behavior we want). I think a user accessing undocumented internal properties is "at your own risk" even if they're unprotected - (hence my hesitation to keep using thix.ctx) :wink:
This is a huge thread with lots of detail, discussion, and nuance. Fascinating to read through.
But, per #5027, we're not looking to significantly change Mocha for the time being. Given that this is doable in userland with wrapper functions or similar, and #1457 tracks adding a plugins API to make these kinds of experiments easier, closing as aged away.
If anybody's reading this and dissapointed in the close, please do file a new issue with a concrete proposal & summary of how you got to that proposal. Cheers!
