-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
🐛 Bug: Contexts not properly reset between tests within the same level #2014
Comments
What are you expecting in terms of behavior? With a simpler example, would you expect this to pass: var assert = require('assert');
describe('Suite', function() {
it('test1', function() {
this.value = 'foo';
assert.equal(this.value, 'foo');
});
it('test2', function() {
assert.notEqual(this.value, 'foo');
});
});
It's always been my understanding that the properties are shared across tests. |
I would expect to not have |
I agree that sharing state between tests is a bad idea :) But it's up to you to clean up state in an afterEach/beforeEach hook. I see these two suites as equal: var assert = require('assert');
describe('Suite1', function() {
it('test1', function() {
this.value = 'foo';
assert.equal(this.value, 'foo');
});
it('test2', function() {
assert.notEqual(this.value, 'foo');
});
});
describe('Suite2', function() {
var value;
it('test1', function() {
value = 'foo';
assert.equal(value, 'foo');
});
it('test2', function() {
assert.notEqual(value, 'foo');
});
});
There's nothing too fancy going on here.
Not sure what you're talking about |
Thanks for the quick response.
That doesn't make sense. Contexts need to be reset automatically. That's their whole point: to save from a mind numbing number of |
Context behavior primary and edge cases have been solved half, if not a full, decade ago in other languages and their libraries. ;-) No need for us to solve the wheel, wrong. The same example from RSpec, passing: describe "Parent" do
before(:each) do @value = 42 end
it "must pass 1" do @value.should == 42 end
it "must pass 2" do @value.should == 42 end
describe "Child" do
before(:each) do
@value.should == 42
@value = 13
end
it "must pass 1" do @value.should == 13 end
it "must pass 2" do @value.should == 13 end
end
end
|
Ah, my mistake, misunderstood the first example. :) PR would be welcome, but it would need to target a major release, since it's a breaking change if done correctly. Might be easier to solve with the mocha 3/4 rewrite. |
Ideally, this solution would have been implemented, rather than merging #1164 As mentioned, any changes to this behavior will be a potentially breaking change. But the current behavior is broken enough that it might just be a bug fix + patch release. |
Good. Looks as if that solution is equivalent to what I proposed almost two years ago. |
And the solution I linked to was proposed in 2012 - no need for snide remarks :) I'm in agreement. |
Ah, indeed. Aimed to be totally neutral. Merely didn't bother with emoticons. :-) |
👍 totally agree with the issue, and that this behaviour should be changed. |
dunno about all of this. if we are going to break the API, I'd like to break it in such a way that functions do not run in a context. |
I would strongly urge users to not use |
Until there's a functional (as opposed to imperative) alternative, it beats reassigning to variables from outer scopes. For one, it prevents accidentally reusing without re-initialization, the thing this bug is about. |
@moll I disagree. I always write all of my tests using function scopes if I need to make a value available. Indeed, this is pretty foolproof: describe('something', () => {
let foo;
beforeEach(() => {
foo = {};
});
it('should do something with foo...', () => {
///
});
}); I've never understood the necessity of using |
related: in some other ticket (forget which) we're talking about the future of the BDD API and whether these callbacks should run in a context (I don't think they should). a potential plan would be
|
(users should still be able to upgrade to latest mocha while keeping their BDD API version held back) |
I don't want to start a holy war, but I've never seen a suite which stores values in |
The current (design) bug isn't because contexts are implemented; it's because they're implemented poorly. ;) I would argue contexts provide stronger isolation. Should you have an async bug in a test, that Contexts are useful for both shared before-eaches and shared tests. E.g. for the former: beforeEach(require("./_signin"))
it("must respond with 200", function*() {
var res = yield request("/", {user: this.user}))
res.statusCode.must.equal(200)
}) Saves writing boilerplate to extract |
Fundamentally, yes, |
I don't see above where |
Imagine the file module.export = function*() {
this.user = yield Users.create()
this.session = yield Sessions.create({user: this.user})
} In that light, my comment of boilerplate might make more sense:
|
@moll I'll concede you can save two or three lines by using
There is, and that's support for lambdas. You should be able to use them in your tests and still be able to control things like how long the test takes to timeout. You should also be free to bind your functions to whatever context you please. So, this is why I want to break the API (eventually). Functions would run w/o contexts, but the first parameter is a example: describe('foo', () => {
it('should complete after awhile', (test, done) => {
test.timeout(10000);
setTimeout(done, 9000);
});
}); But I think I'm way out on a tangent. I think the behavior you have outlined in the original issue is incorrect and should be fixed. Regardless of whether you're using |
Has a merged PR and no further activity in a year, closing. |
@drazisil that PR was merged into yeoman.github.io, not mocha. The issue should probably be left open? :) |
@danielstjules Ah, good catch. Sorry about that and yes. |
I am a bot that watches issues for inactivity. |
I'm sure still am interested in this. |
+1 still interested |
I looked at the original bug, here is the bug, I just changed the names of the test cases to make it more clear: var assert = require('assert');
describe('Parent', function () {
beforeEach(function () {
this.value = 42
});
it('must pass 1', function () {
assert.equal(this.value, 42)
});
it('must pass 2', function () {
assert.equal(this.value, 42)
});
describe('Child', function () {
beforeEach(function () {
assert.equal(this.value, 42);
this.value = 13
});
it('must pass 3', function () {
assert.equal(this.value, 13)
});
it('must pass 4', function () {
assert.equal(this.value, 13) // <<<< this assertion throws
});
});
}); pretty weird bug, don't know how that could be caused |
I think what's happening here is something like:
This is, obviously, rather different from how RSpec was implemented; I haven't gotten around to double-checking how other RSpec-like JavaScript test runners (especially those that claim compatibility with Mocha) treat Recently I was thinking about how Mocha's settings are designed and I wonder if these things might be related. Mocha has a lot of settings like
I'm not sure whether the last point fits, but I wonder if the settings overrides are implemented through a prototype chain too -- and if so, whether these two mechanisms just happen to be similar (perhaps not coincidentally inasmuch as they were both products of the same design pattern) or whether they're actually technically related in the code (which could make it a lot harder to change even if we want to, since we'd have to implement some kind of decoupling between (I haven't actually learned how some of this stuff is implemented really deep under the hood, despite working with Mocha for a couple years now. The bowels of the code are sometimes hard to figure out.) |
Just to remind people, I think I covered the proper implementation of this 3 years ago in #1195. :) |
While I'm in this particular conversation...
You won't find any such thing in Mocha since the suite-level prototype chain ensures that |
I disagree. Please see #3109 I opened a few weeks ago. Prototypical
If something is defined in a non- If the concern is for those that have lost their way off the path of righteousness, I say we help lead them back to the light with a major version bump and that's it. If a medieval cleansing is too cruel, perhaps we even add some documentation explaining why maintaining context is bad . The golden handcuffs of compatibility will either make fixing this inefficient or not really a fix (or both). However, if backwards-compatibility is really a necessity, perhaps we can try:
const staticThis = {};
let localThis = {};
this = new Proxy({}, {
set: (target, property, value) => {
if(magic.isNonEachHook() || magic.isTestCase()) {
localThis[property] = value;
return;
}
staticThis[property] = value;
},
get: (target, property) => {
if(localThis[property]) {
return localThis[property];
}
return staticThis[property];
},
has: (target, property) => {
return !!(localThis[property] || staticThis[property]);
},
deleteProperty: (target, property) => {
delete localThis[property];
delete staticThis[property];
},
defineProperty: (target, property, descriptor) => {
if(magic.isNonEachHook() || magic.isTestCase()) {
return Object.defineProperty(localThis, property, descriptor);
}
return Object.defineProperty(staticThis, property, descriptor);
},
ownKeys: (target, property) => {
return Object.getOwnPropertyNames(staticThis).concat(Object.getOwnPropertyNames(localThis));
}
}); If backwards compatibility is a must, I like the second idea better. Of course, it does leave some room for discussion. For example, how would you handle a Of course, changing something like this will be backwards breaking for some people no matter what (unless we don't actually fix the problem). Those that expect the prototypical I hope all that makes sense. I'll be happy to clarify if necessary. P.S. Please do not take my jokes (imagery) as anything more than that. If I have to write comments, I want to make it fun for the both of us. |
Ok, I looked it up -- what I had seen before (no pun intended) was use of
To clarify, in my mind, the question "can we even fix this without breaking other Mocha suite-nesting behavior to which it may be coupled" is more important than... uh, anything I can imagine a Proxy being useful for in this case; I don't even know whether Proxy is available in all the environments we support at this point.
I'm afraid there's been a misunderstanding -- obviously to "fix" the behavior is going to change the behavior and that will break anything that was relying on the aspects that were changed. But if the fix ever happens, we're going to get multiple/duplicate GitHub issues saying "You broke my suite context in Mocha ! Please fix this to the way it was right before!" (Believe me, I wish I were being cynical and exaggerating, but I'm actually speaking from experience.) Saying "Well we only broke half these things instead of all of them" isn't going to matter. What matters is that we need a more helpful response than "go learn the One True Way to write tests! issue closed!" We need to be able to give them a clear but detailed upgrade path and a succinct but not arbitrary understanding of why it's worthwhile.
Use all the flamboyant imagery you want, if there's a reason to do things one way and not another I would want to hear it; but I have no interest in picking sides in some war over which exact usage of testing tools is the One True Way Just Because the Alternative Happens to Involve State That Isn't Reset Between Tests (hey, if you want to go that far, why not just ban non- |
No need to get defensive, just trying to add my 2-cents and a few suggestions in a funny manner. I will attempt to tone down the humor.
That's a fair point. Proxies weren't supported until Node v6. However, you could always use a polyfill. The point of proxies was to allow support for what (I thought) you were asking: some sort of backward compatibility. By using a Proxy and splitting However, I may have misunderstood what was being asked. In which case, that suggestion can be safely ignored.
That's not what I said ¯\_(ツ)_/¯
Again, a valid point. But, that's what major version bumps are for: breaking changes. If this is too big of a change for anybody, they can simply lock their version into the current major version until they can update their code. It's not ideal, but it happens all the time: Angular, WebPack, Mongoose, etc. In the meantime, educational material should be provided for those that are willing to update.
I would hope the comment would be more helpful. Again, my comment was meant to be playful and exaggerate things. However, it is widely accepted that tests should not 1) be order dependent and 2) maintain state. The former is a requirement for the latter. (I was going to add resources, but I'm much to busy to find them right now for a feature I have a feeling won't be pursued.)
State itself within a test isn't bad. Maintaining state across tests can cause dependencies between tests. Back in my younger days when testing was new to me, my tests were not the best. Many had to be executed in a specific order to set up state for the next test. Now I know why this is bad. One very simple and obvious reason this is bad is that if one of those tests become irrelevant and is removed or simply has to be updated, all tests that come after it now fail. Why do they fail? Not because what they are testing failed, but because state was not maintained. In other words, state across tests couples tests and coupling = bad. The danger of Mocha's
That's a potential solution. It's the "stick your head in the sand" solution, but it is completely valid for Mocha to choose that route. Many frameworks have made decisions that I (and others) disagree with, but guess what, the world keeps turning. Frameworks are meant to be opinionated. If Mocha decides this is one place it takes a stand, more power to you. However, if you decide to go this route, please document the behavior of That being said nobody is saying Mocha is a bad testing framework (I'm certainly not). There's no need to get salty or take what I said personally. What we're saying is we believe we found a bug that may, at one time, have been a feature. We feel the environment has changed and Mocha should change, too. It's perfectly fine for Mocha to decide not to pursue our suggestions; however, if that is the case you need a "clear way to walk people through" this unexpected behavior. Again, Mocha is awesome. It's a very intuitive testing framework. The number of people that use it is a testament to that. |
As the official poster I must admit I don't understand what you all are on about. :P Mountains of text and solutions that span from overengineered Proxies to cloning the world.
What problems? No-one has since talked about mutating the objects in
It solves your example (and mine at the top of this issue) exactly. What part of it doesn't it solve? |
I was told this was a duplicate of the issue I opened (#3109), which describes the problem my developers are facing with the current solution. If this is not the case, these comments should be placed on that issue. If the solution fixes the problems, why is the issue still open? Regardless, I'm going to just have my developers use a |
@c1moore: Just to be clear, no proposed solution is in Mocha's code base today AFAICT, so that explains both you and me experience these problems. |
The use of Given the keystrokes spent on this topic in this and various other issues and PRs, this needs to be distilled to its essence. From what I can gather, there are two main problems with Context:
Does that sound right to everyone? We can use a cigtm-like tool to see just how bad this would break things. If it looks like the impact will be minimal, we could do a major bump for it. But if it's obviously going to break a lot of tests, then no, there is not enough justification nor urgency. "Better docs" is more urgent in either case. I don't really have the time to do the legwork, but if we can assert this won't break the tests of major open-source consumers of Mocha, that'd be a good start. This would require an implementation first, of course! |
@boneskull: That seems to match my understanding. When these issues get addresssed, I'd also ensure the context handover from |
@moll can you please write that example test, just to be sure? |
Will do. |
Hello, I haven't read the whole thing but I found a case when using https://github.com/dareid/chakram, test isolation fails. If, within a I use a Could you advise please? For now, the only solution I found to efficiently help debug is to use the the Thanks! |
@eexit without code, it's tough to help. |
Hey @boneskull, Sorry but I didn't find the time to try to reproduce out of my project. Since it's a proprietary code, I cannot share nothing. |
Hey,
I wouldn't be surprised if related to my years old bug report and gripe: #1195
Fails with:
The text was updated successfully, but these errors were encountered: