Do not apply local configs while testing (#817)

* Do not apply local configs while testing

* Switch to using a command line script

README.md

@@ -59,10 +59,12 @@ API or make any other local changes, just add a local configuration
 file for each app. For example, to run your own discovery pane API, first create
 a local config file:
 
-    cp config/dev-disco.js config/local-disco.js
+    touch config/local-development-disco.js
 
-Override the `apiHost` parameter in `local-disco.js` so that it points to your
-docker container. The file would look like this:
+Be sure to prefix the file with **local-development-** so that it doesn't pollute the
+test suite.
+Here's what `local-development-disco.js` would look like when
+overriding the `apiHost` parameter so that it points to your docker container:
 
 ````javascript
 module.exports = {
@@ -75,6 +77,10 @@ overrides from your local configuration file:
 
     npm run dev:disco
 
+Consult the
+[config file loading order docs](https://github.com/lorenwest/node-config/wiki/Configuration-Files#file-load-order)
+to learn more about how configuration is applied.
+
 ### Building and running services
 
 The following are scripts that are used in deployment - you generally won't

bin/config-check.js

@@ -0,0 +1,34 @@
+#!/usr/bin/env node
+/* eslint-disable global-require, no-console */
+
+// This script exits with an error if there are local configs that could
+// pollute the test suite.
+
+const fs = require('fs');
+const path = require('path');
+
+const chalk = require('chalk');
+
+const root = path.resolve(path.join(path.dirname(__filename), '..'));
+if (!fs.statSync(root).isDirectory()) {
+  throw new Error(`Oops, detected the wrong root? ${root}`);
+}
+
+const configDir = path.join(root, 'config');
+if (!fs.statSync(configDir).isDirectory()) {
+  throw new Error(`Oops, detected the wrong config dir? ${configDir}`);
+}
+
+const disallowedFiles = fs.readdirSync(configDir)
+  // Disallow any local configs except for development configs.
+  .filter((name) =>
+    name.startsWith('local') && !name.startsWith('local-development'))
+  .map((name) => path.join(configDir, name).replace(process.cwd(), '.'));
+
+if (disallowedFiles.length) {
+  console.log(chalk.red(
+    'These local config files are not allowed because they might pollute ' +
+    'the test environment. Prefix them with local-development- instead:'));
+  console.log(chalk.red(disallowedFiles.join('\n')));
+  process.exit(1);
+}

config/test.js

@@ -0,0 +1,2 @@
+// Put any test configuration overrides here.
+module.exports = {};

package.json

@@ -14,11 +14,11 @@
     "eslint": "eslint .",
     "stylelint": "stylelint --syntax scss **/*.scss",
     "lint": "npm run eslint && npm run stylelint",
-    "servertest": "ADDONS_FRONTEND_BUILD_ALL=1 npm run build && better-npm-run servertest && better-npm-run servertest:amo && better-npm-run servertest:disco && better-npm-run servertest:search",
+    "servertest": "bin/config-check.js && ADDONS_FRONTEND_BUILD_ALL=1 npm run build && better-npm-run servertest && better-npm-run servertest:amo && better-npm-run servertest:disco && better-npm-run servertest:search",
     "start": "npm run version-check && NODE_PATH='./:./src' node bin/server.js",
-    "test": "better-npm-run test",
-    "unittest": "better-npm-run unittest",
-    "unittest:dev": "better-npm-run unittest:dev",
+    "test": "bin/config-check.js && better-npm-run test",
+    "unittest": "bin/config-check.js && better-npm-run unittest",
+    "unittest:dev": "bin/config-check.js && better-npm-run unittest:dev",
     "version-check": "node bin/version-check.js",
     "webpack-dev-server": "better-npm-run webpack-dev-server"
   },
@@ -66,7 +66,7 @@
       "command": "mocha --compilers js:babel-register --timeout 10000 tests/server/",
       "env": {
         "NODE_PATH": "./:./src",
-        "NODE_ENV": "production",
+        "NODE_ENV": "test",
         "TRACKING_ENABLED": "false"
       }
     },
@@ -83,7 +83,7 @@
       "command": "mocha --compilers js:babel-register --timeout 10000 tests/server/disco",
       "env": {
         "NODE_PATH": "./:./src",
-        "NODE_ENV": "production",
+        "NODE_ENV": "test",
         "NODE_APP_INSTANCE": "disco",
         "TRACKING_ENABLED": "false"
       }
@@ -92,7 +92,7 @@
       "command": "mocha --compilers js:babel-register --timeout 10000 tests/server/search",
       "env": {
         "NODE_PATH": "./:./src",
-        "NODE_ENV": "production",
+        "NODE_ENV": "test",
         "NODE_APP_INSTANCE": "search",
         "TRACKING_ENABLED": "false"
       }
@@ -101,21 +101,21 @@
       "command": "npm run version-check && npm run unittest && npm run servertest && npm run eslint && npm run stylelint",
       "env": {
         "NODE_PATH": "./:./src",
-        "NODE_ENV": "production"
+        "NODE_ENV": "test"
       }
     },
     "unittest": {
       "command": "karma start --single-run",
       "env": {
         "NODE_PATH": "./:./src",
-        "NODE_ENV": "production"
+        "NODE_ENV": "test"
       }
     },
     "unittest:dev": {
       "command": "karma start",
       "env": {
         "NODE_PATH": "./:./src",
-        "NODE_ENV": "production"
+        "NODE_ENV": "test"
       }
     },
     "webpack-dev-server": {

tests/server/TestConfig.js

@@ -59,15 +59,6 @@ for (const appName of appsList) {
       assert.equal(clientConfig.apiHost, 'https://addons.allizom.org');
       assert.equal(conf.util.getEnv('NODE_ENV'), 'stage');
     });
-
-    it(`should provide a development conf for ${appName}`, () => {
-      process.env.NODE_ENV = 'development';
-      const conf = requireUncached('config');
-      const clientConfig = getClientConfig(conf);
-      assert.equal(conf.get('apiHost'), 'https://addons-dev.allizom.org');
-      assert.equal(clientConfig.apiHost, 'https://addons-dev.allizom.org');
-      assert.equal(conf.util.getEnv('NODE_ENV'), 'development');
-    });
   });
 }