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

greatly improve travis-ci pass rate #6816

Merged
merged 4 commits into from
Aug 6, 2018

Conversation

Fonger
Copy link
Contributor

@Fonger Fonger commented Aug 5, 2018

1. Fix model: querying: with previously existing null values in the db

This test fails frequently. You can see it in almost every travis build that fails.
Like this one: https://travis-ci.org/Automattic/mongoose/jobs/412335127

  1) model: querying:
       with previously existing null values in the db:
      Uncaught AssertionError: 0 == 3
      + expected - actual
      -0
      +3
      
      at Decorator._callFunc (node_modules/empower-core/lib/decorator.js:110:20)
      at Decorator.concreteAssert (node_modules/empower-core/lib/decorator.js:103:17)
      at Function.decoratedAssert [as equal] (node_modules/empower-core/lib/decorate.js:51:30)
      at test/model.querying.test.js:1983:16 ......

Explanation

The description of mocha output is misleading. If you inspect the error stacks, you can find that the actual assertion error is at test/model.querying.test.js:1983:16 so the actual test case that fails is model: querying: with conditionals

I guess it's because the error is thrown in callback so mocha fail to recognize the correct test case. I think tests need to be refactored to avoid throwing error in async callback. We should use done(err) instead to let mocha know where the error is.

There are 10 sub-tests in this test case. So pending tests number should be 10 instead of 9.
The incorrect pending value may lead to Test collection being removed too early and trigger this error. That's why travis-ci often fail on this test because pending value is wrong.

/**
   test/model.querying.test.js Line 1980-1996
*/

      /* ...there are total 10 tests like this, the is is the last test */
      Test.find({block: {$lte: 'buffer shtuffs are neat'}}, function(err, tests) {
        cb();
        assert.ifError(err);

        /** this fail very often because if test collection is cleared
          before all of the tests are finished, tests.length will be 0.
          It leads to `Uncaught AssertionError: 0 == 3`
        */
        assert.equal(tests.length, 3);
      });

      /**
         ⚠️This is incorrect. There are total 10 tests.
      */
      var pending = 9;

      function cb() {
        // When a test is finished, decrement pending number by 1
        if (--pending) {
          return;
        }

        // Clear the Test collection if all pending tests are finished
        Test.remove({}, function(err) {
          assert.ifError(err);
          done();
        });
      }

2. Fix collection name conflict in model: querying: tests

Fix build failure like this: https://travis-ci.org/Automattic/mongoose/jobs/412108300

 4) model: querying:
       update
         can handle minimize option (gh-3381):
     OverwriteModelError: Cannot overwrite `gh3381` model once compiled.
  5) model: querying:
       findOne
         querying if an array contains one of multiple members $in a set:
     Uncaught AssertionError: '5b65c248705df70ab9cd50b4' == 5b65c248705df70ab9cd50b5

Explanation

Simply move the generation of random collection name from before to beforeEach to guarantee unique collection name is used.

before(function(done) {
...
-    collection = 'blogposts_' + random();	
...
});

+  beforeEach(function() {
+    collection = 'blogposts_' + random();
+  });

3. Fix QueryCursor#eachAsync() parallelization and aggregate: cursor() eachAsync with options (parallel)

Fix build failure like this: https://travis-ci.org/Automattic/mongoose/jobs/412101075

  1) QueryCursor
       #eachAsync()
         parallelization:
      AssertionError: false == true
      + expected - actual
      -false
      +true
      
      at test/query.cursor.test.js:332:16

Explanation

If setTimeout is so accurate that delay exactly 100ms, this test will fail.

    it('parallelization', function(done) {
      var cursor = Model.find().sort({ name: 1 }).cursor();

      var names = [];
      var startedAt = [];
      var checkDoc = function(doc) {
        names.push(doc.name);
        startedAt.push(Date.now());
        return {
          then: function(resolve) {
            setTimeout(function() {
              resolve();
            }, 100); // Fired after 100 ms
          }
        };
      };
      cursor.eachAsync(checkDoc, { parallel: 2 }).then(function() {

        // ⚠️This may fail if setTimeout is accurate and delay exactly 100ms
        assert.ok(Date.now() - startedAt[1] > 100);

        assert.equal(startedAt.length, 2);
        assert.ok(startedAt[1] - startedAt[0] < 50);
        assert.deepEqual(names.sort(), ['Axl', 'Slash']);
        done();
      }).catch(done);
    });

@Fonger Fonger changed the title test(cursor): fix eachAsync time check to prevent potential ci failure improve travis-ci pass rate Aug 6, 2018
@Fonger Fonger changed the title improve travis-ci pass rate greatly improve travis-ci pass rate Aug 6, 2018
@Fonger
Copy link
Contributor Author

Fonger commented Aug 6, 2018

EDIT:

At first, I fixed 1. by simply replacing pending with a correct number.

-     var pending = 9;
+     var pending = 10;

However I think we should use a better parallel async tests to prevent future mistake.
If we add a new test and forget to update pending, this will happen again.

Furthermore, mocha reporting wrong test case is an potential issue too

Since we have already used co in a lot of tests, so I refactor it using co

Maybe we need to refactor all of the async test cases to prevent mocha from reporting wrong test case by either

  1. replacing assert.ifError(err); with if (err) return done(err); in callback
  2. using co.catch(done) like this pull request does

For now, I'll recommend to fix this first because it can greatly improve travis-ci pass rate.

All tests pass on my macOS locally and success rate is much higher now.

@Fonger Fonger force-pushed the improve-ci-test branch 2 times, most recently from d919611 to 0ba2d93 Compare August 6, 2018 05:16
@Fonger Fonger force-pushed the improve-ci-test branch 2 times, most recently from 42573f7 to 0b96baa Compare August 6, 2018 09:49
Copy link
Collaborator

@vkarpov15 vkarpov15 left a comment

Choose a reason for hiding this comment

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

Thanks!

@vkarpov15 vkarpov15 merged commit 679279c into Automattic:master Aug 6, 2018
@vkarpov15 vkarpov15 added this to the 5.2.7 milestone Aug 6, 2018
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.

2 participants