fix(ci): flaky tests and dead code

eseliger · SimonSchick · commit 5f9d5907425a · 2019-05-04T19:21:27.000-07:00
diff --git a/lib/dialects/abstract/query.js b/lib/dialects/abstract/query.js
@@ -5,10 +5,12 @@ const SqlString = require('../../sql-string');
 const QueryTypes = require('../../query-types');
 const Dot = require('dottie');
 const deprecations = require('../../utils/deprecations');
+const uuid = require('uuid/v4');
 
 class AbstractQuery {
 
   constructor(connection, sequelize, options) {
+    this.uuid = uuid();
     this.connection = connection;
     this.instance = options.instance;
     this.model = options.model;
diff --git a/lib/dialects/mariadb/query.js b/lib/dialects/mariadb/query.js
@@ -1,7 +1,6 @@
 'use strict';
 
 const AbstractQuery = require('../abstract/query');
-const uuidv4 = require('uuid/v4');
 const sequelizeErrors = require('../../errors');
 const _ = require('lodash');
 const DataTypes = require('../../data-types');
@@ -17,7 +16,6 @@ const debug = logger.debugContext('sql:mariadb');
 class Query extends AbstractQuery {
   constructor(connection, sequelize, options) {
     super(connection, sequelize, Object.assign({ showWarnings: false }, options));
-    this.uuid = uuidv4();
   }
 
   static formatBindParameters(sql, values, dialect) {
diff --git a/lib/dialects/mysql/query.js b/lib/dialects/mysql/query.js
@@ -2,7 +2,6 @@
 
 const Utils = require('../../utils');
 const AbstractQuery = require('../abstract/query');
-const uuidv4 = require('uuid/v4');
 const sequelizeErrors = require('../../errors');
 const _ = require('lodash');
 const { logger } = require('../../utils/logger');
@@ -13,7 +12,6 @@ const debug = logger.debugContext('sql:mysql');
 class Query extends AbstractQuery {
   constructor(connection, sequelize, options) {
     super(connection, sequelize, Object.assign({ showWarnings: false }, options));
-    this.uuid = uuidv4();
   }
 
   static formatBindParameters(sql, values, dialect) {
diff --git a/lib/hooks.js b/lib/hooks.js
@@ -45,7 +45,9 @@ const hookTypes = {
   beforeSync: { params: 1 },
   afterSync: { params: 1 },
   beforeBulkSync: { params: 1 },
-  afterBulkSync: { params: 1 }
+  afterBulkSync: { params: 1 },
+  beforeQuery: { params: 2 },
+  afterQuery: { params: 2 }
 };
 exports.hooks = hookTypes;
 
diff --git a/lib/model.js b/lib/model.js
@@ -1332,7 +1332,7 @@ class Model {
                 changes.push(() => this.QueryInterface.changeColumn(this.getTableName(options), columnName, currentAttribute));
               }
             });
-            return changes.reduce((p, fn) => p.then(fn), Promise.resolve());
+            return Promise.each(changes, f => f());
           });
       })
       .then(() => this.QueryInterface.showIndex(this.getTableName(options), options))
@@ -1349,7 +1349,7 @@ class Model {
           return 0;
         });
 
-        return Promise.map(indexes, index => this.QueryInterface.addIndex(
+        return Promise.each(indexes, index => this.QueryInterface.addIndex(
           this.getTableName(options),
           Object.assign({
             logging: options.logging,
diff --git a/lib/query-interface.js b/lib/query-interface.js
@@ -312,7 +312,7 @@ class QueryInterface {
         });
       }
       return this.getForeignKeysForTables(tableNames, options).then(foreignKeys => {
-        const promises = [];
+        const queries = [];
 
         tableNames.forEach(tableName => {
           let normalizedTableName = tableName;
@@ -321,12 +321,12 @@ class QueryInterface {
           }
 
           foreignKeys[normalizedTableName].forEach(foreignKey => {
-            const sql = this.QueryGenerator.dropForeignKeyQuery(tableName, foreignKey);
-            promises.push(this.sequelize.query(sql, options));
+            queries.push(this.QueryGenerator.dropForeignKeyQuery(tableName, foreignKey));
           });
         });
 
-        return Promise.all(promises).then(() => dropAllTables(tableNames));
+        return Promise.each(queries, q => this.sequelize.query(q, options))
+          .then(() => dropAllTables(tableNames));
       });
     });
   }
diff --git a/lib/sequelize.js b/lib/sequelize.js
@@ -262,17 +262,6 @@ class Sequelize {
 
     this.importCache = {};
 
-    this.test = {
-      _trackRunningQueries: false,
-      _runningQueries: 0,
-      trackRunningQueries() {
-        this._trackRunningQueries = true;
-      },
-      verifyNoRunningQueries() {
-        if (this._runningQueries > 0) throw new Error(`Expected 0 running queries. ${this._runningQueries} queries still running`);
-      }
-    };
-
     Sequelize.runHooks('afterInit', this);
   }
 
@@ -551,17 +540,10 @@ class Sequelize {
 
       const retryOptions = Object.assign({}, this.options.retry, options.retry || {});
 
-      return Promise.resolve(retry(retryParameters => Promise.try(() => {
-        const isFirstTry = retryParameters.current === 1;
-
-        if (isFirstTry && this.test._trackRunningQueries) {
-          this.test._runningQueries++;
-        }
-
+      return Promise.resolve(retry(() => Promise.try(() => {
         if (options.transaction === undefined && Sequelize._cls) {
           options.transaction = Sequelize._cls.get('transaction');
         }
-
         if (options.transaction && options.transaction.finished) {
           const error = new Error(`${options.transaction.finished} has been called on this transaction(${options.transaction.id}), you can no longer use it. (The rejected query is attached as the 'sql' property of this error)`);
           error.sql = sql;
@@ -573,18 +555,15 @@ class Sequelize {
           : this.connectionManager.getConnection(options);
       }).then(connection => {
         const query = new this.dialect.Query(connection, this, options);
-
-        return query.run(sql, bindParameters)
+        return this.runHooks('beforeQuery', options, query)
+          .then(() => query.run(sql, bindParameters))
+          .finally(() => this.runHooks('afterQuery', options, query))
           .finally(() => {
             if (!options.transaction) {
               return this.connectionManager.releaseConnection(connection);
             }
           });
-      }), retryOptions)).finally(() => {
-        if (this.test._trackRunningQueries) {
-          this.test._runningQueries--;
-        }
-      });
+      }), retryOptions));
     });
   }
 
diff --git a/test/integration/model.test.js b/test/integration/model.test.js
@@ -355,7 +355,6 @@ describe(Support.getTestDialectTeaser('Model'), () => {
     });
 
     it('should allow the user to specify indexes in options', function() {
-      this.retries(3);
       const indices = [{
         name: 'a_b_uniq',
         unique: true,
@@ -815,7 +814,7 @@ describe(Support.getTestDialectTeaser('Model'), () => {
 
   describe('save', () => {
     it('should mapping the correct fields when saving instance. see #10589', function() {
-      const User = this.sequelize.define('User', { 
+      const User = this.sequelize.define('User', {
         id3: {
           field: 'id',
           type: Sequelize.INTEGER,
@@ -862,7 +861,7 @@ describe(Support.getTestDialectTeaser('Model'), () => {
     });
 
     it('should mapping the correct fields when updating instance. see #10589', function() {
-      const User = this.sequelize.define('User', { 
+      const User = this.sequelize.define('User', {
         id3: {
           field: 'id',
           type: Sequelize.INTEGER,
diff --git a/test/integration/support.js b/test/integration/support.js
@@ -2,18 +2,26 @@
 
 const Support = require('../support');
 
+const runningQueries = new Set();
+
+before(function() {
+  this.sequelize.addHook('beforeQuery', (options, query) => {
+    runningQueries.add(query);
+  });
+  this.sequelize.addHook('afterQuery', (options, query) => {
+    runningQueries.delete(query);
+  });
+});
+
 beforeEach(function() {
-  this.sequelize.test.trackRunningQueries();
   return Support.clearDatabase(this.sequelize);
 });
 
 afterEach(function() {
-  try {
-    this.sequelize.test.verifyNoRunningQueries();
-  } catch (err) {
-    err.message += ` in ${this.currentTest.fullTitle()}`;
-    throw err;
+  if (runningQueries.size === 0) {
+    return;
   }
+  throw new Error(`Expected 0 running queries. ${runningQueries.size} queries still running in ${this.currentTest.fullTitle()}`);
 });
 
 module.exports = Support;
diff --git a/test/integration/transaction.test.js b/test/integration/transaction.test.js
@@ -124,16 +124,7 @@ if (current.dialect.supports.transactions) {
             };
           // Attention: this test is a bit racy. If you find a nicer way to test this: go ahead
           return SumSumSum.sync({ force: true }).then(() => {
-            return expect(Promise.join(transTest(80), transTest(80), transTest(80))).to.eventually.be.rejectedWith('could not serialize access due to read/write dependencies among transactions');
-          }).delay(100).then(() => {
-            if (this.sequelize.test.$runningQueries !== 0) {
-              return Promise.delay(200);
-            }
-            return void 0;
-          }).then(() => {
-            if (this.sequelize.test.$runningQueries !== 0) {
-              return Promise.delay(500);
-            }
+            return expect(Promise.all([transTest(80), transTest(80), transTest(80)])).to.eventually.be.rejectedWith('could not serialize access due to read/write dependencies among transactions');
           });
         });
       }
diff --git a/test/support.js b/test/support.js
@@ -4,7 +4,6 @@ const fs = require('fs');
 const path = require('path');
 const _ = require('lodash');
 const Sequelize = require('../index');
-const DataTypes = require('../lib/data-types');
 const Config = require('./config/config');
 const chai = require('chai');
 const expect = chai.expect;
@@ -34,53 +33,20 @@ Sequelize.Promise.longStackTraces();
 const Support = {
   Sequelize,
 
-  initTests(options) {
-    const sequelize = this.createSequelizeInstance(options);
-
-    this.clearDatabase(sequelize, () => {
-      if (options.context) {
-        options.context.sequelize = sequelize;
-      }
-
-      if (options.beforeComplete) {
-        options.beforeComplete(sequelize, DataTypes);
-      }
-
-      if (options.onComplete) {
-        options.onComplete(sequelize, DataTypes);
-      }
-    });
-  },
-
-  prepareTransactionTest(sequelize, callback) {
+  prepareTransactionTest(sequelize) {
     const dialect = Support.getTestDialect();
 
     if (dialect === 'sqlite') {
       const p = path.join(__dirname, 'tmp', 'db.sqlite');
+      if (fs.existsSync(p)) {
+        fs.unlinkSync(p);
+      }
+      const options = Object.assign({}, sequelize.options, { storage: p }),
+        _sequelize = new Sequelize(sequelize.config.database, null, null, options);
 
-      return new Sequelize.Promise(resolve => {
-        // We cannot promisify exists, since exists does not follow node callback convention - first argument is a boolean, not an error / null
-        if (fs.existsSync(p)) {
-          resolve(Sequelize.Promise.promisify(fs.unlink)(p));
-        } else {
-          resolve();
-        }
-      }).then(() => {
-        const options = Object.assign({}, sequelize.options, { storage: p }),
-          _sequelize = new Sequelize(sequelize.config.database, null, null, options);
-
-        if (callback) {
-          _sequelize.sync({ force: true }).then(() => { callback(_sequelize); });
-        } else {
-          return _sequelize.sync({ force: true }).return(_sequelize);
-        }
-      });
-    }
-    if (callback) {
-      callback(sequelize);
-    } else {
-      return Sequelize.Promise.resolve(sequelize);
+      return _sequelize.sync({ force: true }).return(_sequelize);
     }
+    return Sequelize.Promise.resolve(sequelize);
   },
 
   createSequelizeInstance(options) {
@@ -165,14 +131,6 @@ const Support = {
       .filter(file => !file.includes('.js') && !file.includes('abstract'));
   },
 
-  checkMatchForDialects(dialect, value, expectations) {
-    if (expectations[dialect]) {
-      expect(value).to.match(expectations[dialect]);
-    } else {
-      throw new Error(`Undefined expectation for "${dialect}"!`);
-    }
-  },
-
   getAbstractQueryGenerator(sequelize) {
     class ModdedQueryGenerator extends AbstractQueryGenerator {
       quoteIdentifier(x) {
@@ -212,24 +170,6 @@ const Support = {
     return `[${dialect.toUpperCase()}] ${moduleName}`;
   },
 
-  getTestUrl(config) {
-    let url;
-    const dbConfig = config[config.dialect];
-
-    if (config.dialect === 'sqlite') {
-      url = `sqlite://${dbConfig.storage}`;
-    } else {
-
-      let credentials = dbConfig.username;
-      if (dbConfig.password) {
-        credentials += `:${dbConfig.password}`;
-      }
-
-      url = `${config.dialect}://${credentials}@${dbConfig.host}:${dbConfig.port}/${dbConfig.database}`;
-    }
-    return url;
-  },
-
   expectsql(query, assertions) {
     const expectations = assertions.query || assertions;
     let expectation = expectations[Support.sequelize.dialect.name];
@@ -261,6 +201,9 @@ const Support = {
 };
 
 if (global.beforeEach) {
+  before(function() {
+    this.sequelize = Support.sequelize;
+  });
   beforeEach(function() {
     this.sequelize = Support.sequelize;
   });
