Automatic merge of gitlab-org/gitlab-ce master

changelogs/unreleased/9928ee-add-rule_type-to-approval-project-rules.yml

+---
+title: Add migration for adding rule_type to approval_project_rules
+merge_request: 30575
+author:
+type: added

db/migrate/20190709204413_add_rule_type_to_approval_project_rules.rb

+# frozen_string_literal: true
+
+class AddRuleTypeToApprovalProjectRules < ActiveRecord::Migration[5.1]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_column_with_default :approval_project_rules, :rule_type, :integer, limit: 2, default: 0, allow_null: false
+  end
+
+  def down
+    remove_column :approval_project_rules, :rule_type
+  end
+end

db/migrate/20190710151229_add_index_to_approval_project_rules_rule_type.rb

+# frozen_string_literal: true
+
+class AddIndexToApprovalProjectRulesRuleType < ActiveRecord::Migration[5.1]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    add_concurrent_index :approval_project_rules, :rule_type
+  end
+
+  def down
+    remove_concurrent_index :approval_project_rules, :rule_type
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 2019_07_15_114644) do
+ActiveRecord::Schema.define(version: 20190715114644) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "pg_trgm"
@@ -282,7 +282,9 @@ ActiveRecord::Schema.define(version: 2019_07_15_114644) do
     t.integer "project_id", null: false
     t.integer "approvals_required", limit: 2, default: 0, null: false
     t.string "name", null: false
+    t.integer "rule_type", limit: 2, default: 0, null: false
     t.index ["project_id"], name: "index_approval_project_rules_on_project_id", using: :btree
+    t.index ["rule_type"], name: "index_approval_project_rules_on_rule_type", using: :btree
   end
 
   create_table "approval_project_rules_groups", force: :cascade do |t|

doc/development/testing_guide/frontend_testing.md

@@ -79,6 +79,34 @@ describe('Component', () => {
 
 Remember that the performance of each test depends on the environment.
 
+### Manual module mocks
+Jest supports [manual module mocks](https://jestjs.io/docs/en/manual-mocks) by placing a mock in a `__mocks__/` directory next to the source module. **Don't do this.** We want to keep all of our test-related code in one place (the `spec/` folder), and the logic that Jest uses to apply mocks from `__mocks__/` is rather inconsistent.
+
+Instead, our test runner detects manual mocks from `spec/frontend/mocks/`. Any mock placed here is automatically picked up and injected whenever you import its source module.
+
+- Files in `spec/frontend/mocks/ce` will mock the corresponding CE module from `app/assets/javascripts`, mirroring the source module's path.
+    - Example: `spec/frontend/mocks/ce/lib/utils/axios_utils` will mock the module `~/lib/utils/axios_utils`.
+- Files in `spec/frontend/mocks/node` will mock NPM packages of the same name or path.
+- We don't support mocking EE modules yet.
+
+If a mock is found for which a source module doesn't exist, the test suite will fail. 'Virtual' mocks, or mocks that don't have a 1-to-1 association with a source module, are not supported yet.
+
+#### Writing a mock
+Create a JS module in the appropriate place in `spec/frontend/mocks/`. That's it. It will automatically mock its source package in all tests.
+
+Make sure that your mock's export has the same format as the mocked module. So, if you're mocking a CommonJS module, you'll need to use `module.exports` instead of the ES6 `export`.
+
+It might be useful for a mock to expose a property that indicates if the mock was loaded. This way, tests can assert the presence of a mock without calling any logic and causing side-effects. The `~/lib/utils/axios_utils` module mock has such a property, `isMock`, that is `true` in the mock and undefined in the original class. Jest's mock functions also have a `mock` property that you can test.
+
+#### Bypassing mocks
+If you ever need to import the original module in your tests, use [`jest.requireActual()`](https://jestjs.io/docs/en/jest-object#jestrequireactualmodulename) (or `jest.requireActual().default` for the default export). The `jest.mock()` and `jest.unmock()` won't have an effect on modules that have a manual mock, because mocks are imported and cached before any tests are run.
+
+#### Keep mocks light
+Global mocks introduce magic and can affect how modules are imported in your tests. Try to keep them as light as possible and dependency-free. A global mock should be useful for any unit test. For example, the `axios_utils` and `jquery` module mocks throw an error when an HTTP request is attempted, since this is useful behaviour in >99% of tests.
+
+When in doubt, construct mocks in your test file using [`jest.mock()`](https://jestjs.io/docs/en/jest-object#jestmockmodulename-factory-options), [`jest.spyOn()`](https://jestjs.io/docs/en/jest-object#jestspyonobject-methodname), etc.
+
+
 ## Karma test suite
 
 GitLab uses the [Karma][karma] test runner with [Jasmine] as its test

package.json

@@ -191,6 +191,7 @@
     "pixelmatch": "^4.0.2",
     "postcss": "^7.0.14",
     "prettier": "1.18.2",
+    "readdir-enhanced": "^2.2.4",
     "stylelint": "^9.10.1",
     "stylelint-config-recommended": "^2.1.0",
     "stylelint-scss": "^3.5.4",

spec/frontend/mocks/ce/lib/utils/axios_utils.js

+const axios = jest.requireActual('~/lib/utils/axios_utils').default;
+
+axios.isMock = true;
+
+// Fail tests for unmocked requests
+axios.defaults.adapter = config => {
+  const message =
+    `Unexpected unmocked request: ${JSON.stringify(config, null, 2)}\n` +
+    'Consider using the `axios-mock-adapter` in tests.';
+  const error = new Error(message);
+  error.config = config;
+  throw error;
+};
+
+export default axios;

spec/frontend/mocks/mocks_helper.js

+/**
+ * @module
+ *
+ * This module implements auto-injected manual mocks that are cleaner than Jest's approach.
+ *
+ * See https://docs.gitlab.com/ee/development/testing_guide/frontend_testing.html
+ */
+
+import fs from 'fs';
+import path from 'path';
+
+import readdir from 'readdir-enhanced';
+
+const MAX_DEPTH = 20;
+const prefixMap = [
+  // E.g. the mock ce/foo/bar maps to require path ~/foo/bar
+  { mocksRoot: 'ce', requirePrefix: '~' },
+  // { mocksRoot: 'ee', requirePrefix: 'ee' }, // We'll deal with EE-specific mocks later
+  { mocksRoot: 'node', requirePrefix: '' },
+  // { mocksRoot: 'virtual', requirePrefix: '' }, // We'll deal with virtual mocks later
+];
+
+const mockFileFilter = stats => stats.isFile() && stats.path.endsWith('.js');
+
+const getMockFiles = root => readdir.sync(root, { deep: MAX_DEPTH, filter: mockFileFilter });
+
+// Function that performs setting a mock. This has to be overridden by the unit test, because
+// jest.setMock can't be overwritten across files.
+// Use require() because jest.setMock expects the CommonJS exports object
+const defaultSetMock = (srcPath, mockPath) =>
+  jest.mock(srcPath, () => jest.requireActual(mockPath));
+
+// eslint-disable-next-line import/prefer-default-export
+export const setupManualMocks = function setupManualMocks(setMock = defaultSetMock) {
+  prefixMap.forEach(({ mocksRoot, requirePrefix }) => {
+    const mocksRootAbsolute = path.join(__dirname, mocksRoot);
+    if (!fs.existsSync(mocksRootAbsolute)) {
+      return;
+    }
+
+    getMockFiles(path.join(__dirname, mocksRoot)).forEach(mockPath => {
+      const mockPathNoExt = mockPath.substring(0, mockPath.length - path.extname(mockPath).length);
+      const sourcePath = path.join(requirePrefix, mockPathNoExt);
+      const mockPathRelative = `./${path.join(mocksRoot, mockPathNoExt)}`;
+
+      try {
+        setMock(sourcePath, mockPathRelative);
+      } catch (e) {
+        if (e.message.includes('Could not locate module')) {
+          // The corresponding mocked module doesn't exist. Raise a better error.
+          // Eventualy, we may support virtual mocks (mocks whose path doesn't directly correspond
+          // to a module, like with the `ee_else_ce` prefix).
+          throw new Error(
+            `A manual mock was defined for module ${sourcePath}, but the module doesn't exist!`,
+          );
+        }
+      }
+    });
+  });
+};

spec/frontend/mocks/mocks_helper_spec.js

+/* eslint-disable global-require, promise/catch-or-return */
+
+import path from 'path';
+
+import axios from '~/lib/utils/axios_utils';
+
+const absPath = path.join.bind(null, __dirname);
+
+jest.mock('fs');
+jest.mock('readdir-enhanced');
+
+describe('mocks_helper.js', () => {
+  let setupManualMocks;
+  const setMock = jest.fn().mockName('setMock');
+  let fs;
+  let readdir;
+
+  beforeAll(() => {
+    jest.resetModules();
+    jest.setMock = jest.fn().mockName('jest.setMock');
+    fs = require('fs');
+    readdir = require('readdir-enhanced');
+
+    // We need to provide setupManualMocks with a mock function that pretends to do the setup of
+    // the mock. This is because we can't mock jest.setMock across files.
+    setupManualMocks = () => require('./mocks_helper').setupManualMocks(setMock);
+  });
+
+  afterEach(() => {
+    fs.existsSync.mockReset();
+    readdir.sync.mockReset();
+    setMock.mockReset();
+  });
+
+  it('enumerates through mock file roots', () => {
+    setupManualMocks();
+    expect(fs.existsSync).toHaveBeenCalledTimes(2);
+    expect(fs.existsSync).toHaveBeenNthCalledWith(1, absPath('ce'));
+    expect(fs.existsSync).toHaveBeenNthCalledWith(2, absPath('node'));
+
+    expect(readdir.sync).toHaveBeenCalledTimes(0);
+  });
+
+  it("doesn't traverse the directory tree infinitely", () => {
+    fs.existsSync.mockReturnValue(true);
+    readdir.sync.mockReturnValue([]);
+    setupManualMocks();
+
+    readdir.mock.calls.forEach(call => {
+      expect(call[1].deep).toBeLessThan(100);
+    });
+  });
+
+  it('sets up mocks for CE (the ~/ prefix)', () => {
+    fs.existsSync.mockImplementation(root => root.endsWith('ce'));
+    readdir.sync.mockReturnValue(['root.js', 'lib/utils/util.js']);
+    setupManualMocks();
+
+    expect(readdir.sync).toHaveBeenCalledTimes(1);
+    expect(readdir.sync.mock.calls[0][0]).toBe(absPath('ce'));
+
+    expect(setMock).toHaveBeenCalledTimes(2);
+    expect(setMock).toHaveBeenNthCalledWith(1, '~/root', './ce/root');
+    expect(setMock).toHaveBeenNthCalledWith(2, '~/lib/utils/util', './ce/lib/utils/util');
+  });
+
+  it('sets up mocks for node_modules', () => {
+    fs.existsSync.mockImplementation(root => root.endsWith('node'));
+    readdir.sync.mockReturnValue(['jquery', '@babel/core']);
+    setupManualMocks();
+
+    expect(readdir.sync).toHaveBeenCalledTimes(1);
+    expect(readdir.sync.mock.calls[0][0]).toBe(absPath('node'));
+
+    expect(setMock).toHaveBeenCalledTimes(2);
+    expect(setMock).toHaveBeenNthCalledWith(1, 'jquery', './node/jquery');
+    expect(setMock).toHaveBeenNthCalledWith(2, '@babel/core', './node/@babel/core');
+  });
+
+  it('sets up mocks for all roots', () => {
+    const files = {
+      [absPath('ce')]: ['root', 'lib/utils/util'],
+      [absPath('node')]: ['jquery', '@babel/core'],
+    };
+
+    fs.existsSync.mockReturnValue(true);
+    readdir.sync.mockImplementation(root => files[root]);
+    setupManualMocks();
+
+    expect(readdir.sync).toHaveBeenCalledTimes(2);
+    expect(readdir.sync.mock.calls[0][0]).toBe(absPath('ce'));
+    expect(readdir.sync.mock.calls[1][0]).toBe(absPath('node'));
+
+    expect(setMock).toHaveBeenCalledTimes(4);
+    expect(setMock).toHaveBeenNthCalledWith(1, '~/root', './ce/root');
+    expect(setMock).toHaveBeenNthCalledWith(2, '~/lib/utils/util', './ce/lib/utils/util');
+    expect(setMock).toHaveBeenNthCalledWith(3, 'jquery', './node/jquery');
+    expect(setMock).toHaveBeenNthCalledWith(4, '@babel/core', './node/@babel/core');
+  });
+
+  it('fails when given a virtual mock', () => {
+    fs.existsSync.mockImplementation(p => p.endsWith('ce'));
+    readdir.sync.mockReturnValue(['virtual', 'shouldntBeImported']);
+    setMock.mockImplementation(() => {
+      throw new Error('Could not locate module');
+    });
+
+    expect(setupManualMocks).toThrow(
+      new Error("A manual mock was defined for module ~/virtual, but the module doesn't exist!"),
+    );
+
+    expect(readdir.sync).toHaveBeenCalledTimes(1);
+    expect(readdir.sync.mock.calls[0][0]).toBe(absPath('ce'));
+  });
+
+  describe('auto-injection', () => {
+    it('handles ambiguous paths', () => {
+      jest.isolateModules(() => {
+        const axios2 = require('../../../app/assets/javascripts/lib/utils/axios_utils').default;
+        expect(axios2.isMock).toBe(true);
+      });
+    });
+
+    it('survives jest.isolateModules()', done => {
+      jest.isolateModules(() => {
+        const axios2 = require('~/lib/utils/axios_utils').default;
+        expect(axios2.get('http://gitlab.com'))
+          .rejects.toThrow('Unexpected unmocked request')
+          .then(done);
+      });
+    });
+
+    it('can be unmocked and remocked', () => {
+      jest.dontMock('~/lib/utils/axios_utils');
+      jest.resetModules();
+      const axios2 = require('~/lib/utils/axios_utils').default;
+      expect(axios2).not.toBe(axios);
+      expect(axios2.isMock).toBeUndefined();
+
+      jest.doMock('~/lib/utils/axios_utils');
+      jest.resetModules();
+      const axios3 = require('~/lib/utils/axios_utils').default;
+      expect(axios3).not.toBe(axios2);
+      expect(axios3.isMock).toBe(true);
+    });
+  });
+});

spec/frontend/mocks/node/jquery.js

+/* eslint-disable import/no-commonjs */
+
+const $ = jest.requireActual('jquery');
+
+// Fail tests for unmocked requests
+$.ajax = () => {
+  throw new Error(
+    'Unexpected unmocked jQuery.ajax() call! Make sure to mock jQuery.ajax() in tests.',
+  );
+};
+
+// jquery is not an ES6 module
+module.exports = $;

spec/frontend/mocks_spec.js

+import $ from 'jquery';
+import axios from '~/lib/utils/axios_utils';
+
+describe('Mock auto-injection', () => {
+  describe('mocks', () => {
+    it('~/lib/utils/axios_utils', () =>
+      expect(axios.get('http://gitlab.com')).rejects.toThrow('Unexpected unmocked request'));
+
+    it('jQuery.ajax()', () => {
+      expect($.ajax).toThrow('Unexpected unmocked');
+    });
+  });
+});

spec/frontend/operation_settings/components/external_dashboard_spec.js

@@ -7,7 +7,6 @@ import { refreshCurrentPage } from '~/lib/utils/url_utility';
 import createFlash from '~/flash';
 import { TEST_HOST } from 'helpers/test_constants';
 
-jest.mock('~/lib/utils/axios_utils');
 jest.mock('~/lib/utils/url_utility');
 jest.mock('~/flash');
 
@@ -32,6 +31,10 @@ describe('operation settings external dashboard component', () => {
     wrapper = shallow ? shallowMount(...config) : mount(...config);
   };
 
+  beforeEach(() => {
+    jest.spyOn(axios, 'patch').mockImplementation();
+  });
+
   afterEach(() => {
     if (wrapper.destroy) {
       wrapper.destroy();

spec/frontend/test_setup.js

@@ -2,10 +2,10 @@ import Vue from 'vue';
 import * as jqueryMatchers from 'custom-jquery-matchers';
 import $ from 'jquery';
 import Translate from '~/vue_shared/translate';
-import axios from '~/lib/utils/axios_utils';
 import { config as testUtilsConfig } from '@vue/test-utils';
 import { initializeTestTimeout } from './helpers/timeout';
 import { loadHTMLFixture, setHTMLFixture } from './helpers/fixtures';
+import { setupManualMocks } from './mocks/mocks_helper';
 
 // Expose jQuery so specs using jQuery plugins can be imported nicely.
 // Here is an issue to explore better alternatives:
@@ -14,6 +14,8 @@ window.jQuery = $;
 
 process.on('unhandledRejection', global.promiseRejectionHandler);
 
+setupManualMocks();
+
 afterEach(() =>
   // give Promises a bit more time so they fail the right test
   new Promise(setImmediate).then(() => {
@@ -24,18 +26,6 @@ afterEach(() =>
 
 initializeTestTimeout(process.env.CI ? 5000 : 500);
 
-// fail tests for unmocked requests
-beforeEach(done => {
-  axios.defaults.adapter = config => {
-    const error = new Error(`Unexpected unmocked request: ${JSON.stringify(config, null, 2)}`);
-    error.config = config;
-    done.fail(error);
-    return Promise.reject(error);
-  };
-
-  done();
-});
-
 Vue.config.devtools = false;
 Vue.config.productionTip = false;
 

yarn.lock

@@ -4919,6 +4919,11 @@ glob-to-regexp@^0.3.0:
   resolved "https://registry.yarnpkg.com/glob-to-regexp/-/glob-to-regexp-0.3.0.tgz#8c5a1494d2066c570cc3bfe4496175acc4d502ab"
   integrity sha1-jFoUlNIGbFcMw7/kSWF1rMTVAqs=
 
+glob-to-regexp@^0.4.0:
+  version "0.4.1"
+  resolved "https://registry.yarnpkg.com/glob-to-regexp/-/glob-to-regexp-0.4.1.tgz#c75297087c851b9a578bd217dd59a92f59fe546e"
+  integrity sha512-lkX1HJXwyMcprw/5YUZc2s7DrpAiHB21/V+E1rHUrVNokkvB6bqMzT0VfV6/86ZNabt1k14YOIaT7nDvOX3Iiw==
+
 "glob@5 - 7", glob@^7.0.0, glob@^7.0.3, glob@^7.1.1, glob@^7.1.2, glob@^7.1.3, glob@~7.1.1:
   version "7.1.4"
   resolved "https://registry.yarnpkg.com/glob/-/glob-7.1.4.tgz#aa608a2f6c577ad357e1ae5a5c26d9a8d1969255"
@@ -9099,6 +9104,14 @@ readable-stream@~2.0.6:
     string_decoder "~0.10.x"
     util-deprecate "~1.0.1"
 
+readdir-enhanced@^2.2.4:
+  version "2.2.4"
+  resolved "https://registry.yarnpkg.com/readdir-enhanced/-/readdir-enhanced-2.2.4.tgz#773fb8a8de5f645fb13d9403746d490d4facb3e6"
+  integrity sha512-JQD83C9gAs5B5j2j40qLn/K83HhR8po3bUonebNeuJQUZbbn7q1HxL9kQuPBtxoXkaUpbtEmpFBw5kzyYnnJDA==
+  dependencies:
+    call-me-maybe "^1.0.1"
+    glob-to-regexp "^0.4.0"
+
 readdirp@^2.0.0:
   version "2.1.0"
   resolved "https://registry.yarnpkg.com/readdirp/-/readdirp-2.1.0.tgz#4ed0ad060df3073300c48440373f72d1cc642d78"