Skip to content

Conversation

@bajtos
Copy link
Member

@bajtos bajtos commented Jun 12, 2018

  • Drop support for Node 4.x
  • Travis: add Node.js 8.x + 10.x to the build matrix
  • Disable package-lock feature of npm
  • Update eslint + config to latest
  • Update Mocha and Chai to latest
  • Update strong-globalize to 4.x
  • Update msgpack5 to 4.x

@bajtos bajtos self-assigned this Jun 12, 2018
@bajtos bajtos requested review from a team and raymondfeng June 12, 2018 14:54
beforeEach(createPostInTx(post, TIMEOUT));

it('should report timeout', function(done) {
setTimeout(function() {
Copy link
Member Author

Choose a reason for hiding this comment

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

This test did not make any sense to me. The original code:

    it('should report timeout', function(done) {
      setTimeout(function() {
        Post.find({where: {title: 't3'}}, {transaction: currentTx},
          function(err, posts) {
            if (err) return done(err);
            expect(posts.length).to.be.eql(1);
            done();
          });
      }, 300);
      done();
    });

Notice the done() callback is called immediately after setTimeout, before setTimeout fires. As a result, the test was marked as passed by mocha and because the remaining tests finished before the timeout fired, the process exited.

Mocha no longer exits the process when tests finish, in which case the setTimeout callback has time to be fired and fails with an error - this error is reported outside of any test case though. What a mess!

I rewrote the test as follows, I hope I captured author's intent correctly (ping @raymondfeng):

    it('should report timeout', function(done) {
      // wait until the "create post" transaction times out
      setTimeout(runTheTest, TIMEOUT * 3);

      function runTheTest() {
        Post.find({where: {title: 't3'}}, {transaction: currentTx},
          function(err, posts) {
            expect(err).to.match(/transaction.*not active/);
            done();
          });
      }
    });

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch.

Copy link
Contributor

@jannyHou jannyHou left a comment

Choose a reason for hiding this comment

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

@bajtos LGTM 👍 just one nitpick about the test


// If the event is not fired quickly enough, then the test can
// quickly fail - no need to wait full two seconds (Mocha's default)
this.timeout(TIMEOUT * 3);
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: wouldn't it be set before test 'should report timeout'?

Copy link
Member Author

Choose a reason for hiding this comment

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

The timeout function must be called from the test/hook that it is supposed to apply to. See https://mochajs.org/#timeouts

If called before the test (it call), it would apply to the entire test suite (describe block).

@bajtos bajtos merged commit 45cbee7 into master Jun 14, 2018
@bajtos bajtos deleted the update-deps branch June 14, 2018 07:33
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.

7 participants