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

this using inheritance causes problems #2977

Closed
tsclaus opened this issue Sep 1, 2017 · 2 comments
Closed

this using inheritance causes problems #2977

tsclaus opened this issue Sep 1, 2017 · 2 comments
Labels
type: question support question

Comments

@tsclaus
Copy link

tsclaus commented Sep 1, 2017

The last it will unexpectedly fail.

describe.only('Example', function() {
  beforeEach(function() {
    this.value = 5;
  });

  it('should have the right value', function() {
    this.value.should.equal(5);

    this.value = 'who cares';
  });

  it('should still have the right value', function() {
    this.value.should.equal(5);

    this.value = 'who cares';
  });

  describe('Inner', function() {
    it('should have the right value', function() {
      this.value.should.equal(5);

      this.value = 'who cares';
    });

    it('should still have the right value but won\'t', function() {
      this.value.should.equal(5);
    });
  })
});

Using console.log, I have verified the beforeEach is running before each and every test and it is, so the only way this could occur is if the this in the Inner tests is different from the one in the outer tests and the beforeEach. Since values are still accessible, I am guessing Inner blocks' this prototypically inherits from the outer block. If that is the case, why? Suites are never running simultaneously, so how would injecting the exact same object as this regardless of nesting cause an error? FWIW, Karma, which it seems like your this is based off of, doesn't use inheritance.

It also seems dangerous that you're reusing this but that might be a performance thing; to make tests more isolated from each other I would expect a new this to be instantiated for each and every it but that's not the case given this error.

@ScottFreeCode
Copy link
Contributor

We have multiple issues already open about this not being reset before every test and this behaving different from how users expect with regard to nested suites...

Mocha's this behavior is actually something like twice as old as Karma's whole existence (maybe more) and -- although I could be wrong on this point -- I don't think anyone currently on the team was around when it was designed to remember why it is the way it is (or at least, the one current member who joined on before the original developer left doesn't happen to know any good use case for this at all, if I haven't misread his comments on other issues about it). In fact, as far as I can tell there doesn't seem to be any particular advantage of the existing design over using local variables in the suite callbacks -- but, Mocha's massive userbase being what it is, there are certainly people depending on it...

Personally, I would like it to be more RSpec-like instead (reset before every test automatically, more useful and less confusing nested suite behavior that can't easily be achieved with local variables), but I'm not up for making that happen on my own, so we'd need some team consensus on it (and really, we'd need team consensus on braving the inevitable upset of some users that we changed it even if the change is justified and happens in a semver "major" release). Ultimately, unless the rest of the team ends up convinced by one case or another for a change, what I'm going to say is same thing I said to the last couple of requests about it: To get a full picture of what the pros and cons of a change are, in addition to knowing on the one hand that any change is going to cause a certain amount of backwards-compatibility backlash and on the other hand the various cases to be made for more useful behavior and that the current behavior surprises some users, is useless (or very nearly so) and doesn't match RSpec, the one last piece of info we'd need is what other RSpec-like JavaScript test runners do (especially ones that claim compatibility with Mocha).

@boneskull
Copy link
Contributor

boneskull commented Oct 4, 2017

yes, don't use this for storing data created in a hook; use closures instead. this buys you nothing except garmonbozia (pain and sorrow)

the context is tied to the suite, and it's passed around... I couldn't tell you why it's based on the suite (unless I have a whole lot of time to kill), but as @ScottFreeCode, I don't think we can really monkey with it. given that I have yet to see this particular error before, it seems others are doing a good job of avoiding this gotcha.

so, yeah, it's not an ideal architecture. but don't use this and you're golden:

describe.only('Example', function() {
  let value;
  
  beforeEach(function() {
    value = 5;
  });

  it('should have the right value', function() {
    value.should.equal(5);

    value = 'who cares';
  });

  it('should still have the right value', function() {
    value.should.equal(5);

    value = 'who cares';
  });

  describe('Inner', function() {
    it('should have the right value', function() {
     value.should.equal(5);

      value = 'who cares';
    });

    it('should still have the right value but won\'t', function() {
      value.should.equal(5); // success
    });
  })
});

(BTW, Karma has no test framework of its own; it's just a runner. You may be thinking of Jasmine.)

@boneskull boneskull added the type: question support question label Oct 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: question support question
Projects
None yet
Development

No branches or pull requests

3 participants