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

Nested this Not Reset for Every Test #3109

Closed
4 tasks done
c1moore opened this issue Nov 16, 2017 · 2 comments
Closed
4 tasks done

Nested this Not Reset for Every Test #3109

c1moore opened this issue Nov 16, 2017 · 2 comments

Comments

@c1moore
Copy link

c1moore commented Nov 16, 2017

Prerequisites

  • Checked that your issue isn't already filed by cross referencing issues with the common mistake label
    Doesn't appear to be any other issues like this, except this one.
  • Checked next-gen ES issues and syntax problems by using the same environment and/or transpiler configuration without Mocha to ensure it isn't just a feature that actually isn't supported in the environment in question or a bug in your code.
    Not using any of that fancy stuff.
  • 'Smoke tested' the code to be tested by running it outside the real test suite to get a better sense of whether the problem is in the code under test, your usage of Mocha, or Mocha itself
    Tested completely separate from rest of system.
  • Ensured that there is no discrepancy between the locally and globally installed versions of Mocha. You can find them with:
    node node_modules/.bin/mocha --version(Local) and mocha --version(Global). We recommend avoiding the use of globally installed Mocha.
    Don't have a globally installed version of mocha.

Description

When setting a value in an outer describe block on this in a beforeEach, the value should be reset for all tests defined within this describe block and all nested describe blocks. However, this is not the case. Nested describe block maintain their own state. This is noticeable when attempting to append a string to the end of the value defined in the outer describe block.

Use Case: Appending a string to the end of a variable defined on this is extremely useful for testing, especially routes. For instance, if this worked, I could create tests for my API like so:

describe('API Integration Tests', function() {
  beforeEach(function() {
    this.route = '';
  });

  describer('User Endpoints', function() {
    beforeEach(function() {
      this.route += '/users';

      this.user = new User();
    });

    describe("User's Children Endpoints", function() {
      beforeEach(function() {
        this.route += ('/' + user.id + '/children');
      });
    });
  });
});

Steps to Reproduce

Copy/paste the following code and run. 1 test (the last one) will fail.

'use strict';

describe.only('Outer Block', function() {
  beforeEach(function() {
    this.myVar = 'myVar';
  });

  it('should equal myVar', function() {
    this.myVar.should.equal('myVar');
  });

  describe('Inner Block', function() {
    beforeEach(function() {
      this.myVar += ' is the best!';
    });

    it('should equal "myVar is the best!"', function() {
      this.myVar.should.equal('myVar is the best!');
    });

    it('should equal "myVar is the best!"', function() {
      this.myVar.should.equal('myVar is the best!');
    });
  });
});

Expected behavior: [What you expect to happen]

All tests should pass. This would mean the inner this.myVar is reset to equal the string "myVar" (the value it is being set to in the outer beforeEach) before every test.

Actual behavior: [What actually happens]

The last test is failing because this.myVar refers to the value of this.myVar from the last test. I.e. mocha maintains state across tests 😱

Reproduces how often: [What percentage of the time does it reproduce?] Every time

Versions

Shouldn't be necessary, but

  • mocha --version: The program 'mocha' is currently not installed. You can install it by typing:
    sudo apt install mocha
  • node node_modules/.bin/mocha --version: 4.0.1
  • node --version: 8.9.1

The rest seems unnecessary.

Possible Solutions

Set this to an empty object ({}) for every test.

@c1moore
Copy link
Author

c1moore commented Nov 16, 2017

Immediately after posting this, I found some other issues around this for Mocha. Seems the consensus is "won't fix". I'll leave this open for commenting, but given that Mocha already almost follows this behavior, it seems it would be appropriate to fix this gotcha.

I suppose the workaround is using variables defined within the describe block. This is how I'm used to working with Mocha since I was using it long before this worked like it does today, but as our code base does start to transition toward using this, little bugs like this pop up and kind of bite us in the butt.

@ScottFreeCode
Copy link
Contributor

Closing as a duplicate of #2014, but feel free to weigh in there. I summarized my understanding in #2014 (comment)

I don't know about the rest of the team, but I actually would like to see this "fixed" -- give this behavior that is actually useful in a way describe-level var/let/const isn't -- however, I'd like to get a better survey of how it affects compatibility with other JS test runners and some idea of how easily we can direct people to correct any backwards-compatibility issues it would cause if their tests are leveraging the current behavior (even granting the assumption that they probably shouldn't be)... and I don't know how complicated it will be to implement in the event that we do settle on some improvements.

(Additionally, one factor I'd like to look at is whether one can have per-test data/actions/resources that are set using suite-level data, i.e. using data that can be overridden in nested suites. For example, it would be neat if it were possible for a before to set a value this.x and a nested suite's before to update it while the outer suite's beforeEach uses this.x and gets the outer suite's value when running before tests in the outer suite and the inner suite's value when running before tests in the inner suite. I guess that'd involve cloning this instead of prototype-chaining it and passing the current suite's this to Each hooks instead of the this associated with their specific suite. Anyway, I've been told that RSpec allows doing something to achieve such an effect, although I don't recall if it was with hooks in the first place or with some other mechanism -- I'd have to go look up that discussion.)

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

No branches or pull requests

2 participants