Overhaul initialization pattern for server + server tests (#2519)

Because `server.js` was long a monolith, there are a bunch of shims in place to facilitate unit testing. A few of the test suites share port 1111 which means if one of them fails to set up, the port won't be freed and other unrelated tests will fail. Some of the tests which trigger server setup include timeouts which were added to give setup code time to run. In one the test suites, we actually modify `process.argv`, which seems completely gross.

This implements a few changes which improve this:

1. Separate the server from the server startup script, splitting out `lib/server.js`.
2. Inject config into the server and validate the config schema.
3. Inject config into the service test runner.
4. Use `portfinder`, a popular utility for grabbing open ports during testing.
5. Switch more of the setup code from callbacks to async-await.

Overall it leaves everything acting more reliably and looking rather cleaner, if in a few places more verbose.

It also fixes the root cause of #1455, a `setTimeout` in `rate-limit`. Off and on during development of this changeset, Mocha would decide not to exit, and that turned out to be the culprit.

Fix #1455

lib/sys/monitor.js

@@ -2,7 +2,6 @@
 
 const secretIsValid = require('./secret-is-valid')
 const serverSecrets = require('../server-secrets')
-const config = require('../server-config')
 const RateLimit = require('./rate-limit')
 const log = require('../log')
 
@@ -17,7 +16,7 @@ function secretInvalid(req, res) {
   return false
 }
 
-function setRoutes(server) {
+function setRoutes({ rateLimit }, server) {
   const ipRateLimit = new RateLimit({
     whitelist: /^192\.30\.252\.\d+$/, // Whitelist GitHub IPs.
   })
@@ -34,7 +33,7 @@ function setRoutes(server) {
       }
     }
 
-    if (config.rateLimit) {
+    if (rateLimit) {
       const ip =
         (req.headers['x-forwarded-for'] || '').split(', ')[0] ||
         req.socket.remoteAddress
@@ -86,6 +85,12 @@ function setRoutes(server) {
       referer: refererRateLimit.toJSON(),
     })
   })
+
+  return function() {
+    ipRateLimit.stop()
+    badgeTypeRateLimit.stop()
+    refererRateLimit.stop()
+  }
 }
 
 module.exports = {

lib/sys/prometheus-metrics.spec.js

@@ -2,32 +2,31 @@
 
 const { expect } = require('chai')
 const Camp = require('camp')
+const portfinder = require('portfinder')
 const fetch = require('node-fetch')
-const config = require('../test-config')
 const Metrics = require('./prometheus-metrics')
 
 describe('Prometheus metrics route', function() {
-  const baseUrl = `http://127.0.0.1:${config.port}`
+  let port, baseUrl
+  beforeEach(async function() {
+    port = await portfinder.getPortPromise()
+    baseUrl = `http://127.0.0.1:${port}`
+  })
 
   let camp
-  afterEach(function(done) {
+  beforeEach(async function() {
+    camp = Camp.start({ port, hostname: '::' })
+    await new Promise(resolve => camp.on('listening', () => resolve()))
+  })
+  afterEach(async function() {
     if (camp) {
-      camp.close(() => done())
+      await new Promise(resolve => camp.close(resolve))
       camp = undefined
     }
   })
 
-  function startServer(metricsConfig) {
-    return new Promise((resolve, reject) => {
-      camp = Camp.start({ port: config.port, hostname: '::' })
-      const metrics = new Metrics(metricsConfig)
-      metrics.initialize(camp)
-      camp.on('listening', () => resolve())
-    })
-  }
-
   it('returns 404 when metrics are disabled', async function() {
-    startServer({ enabled: false })
+    new Metrics({ enabled: false }).initialize(camp)
 
     const res = await fetch(`${baseUrl}/metrics`)
 
@@ -36,7 +35,7 @@ describe('Prometheus metrics route', function() {
   })
 
   it('returns 404 when there is no configuration', async function() {
-    startServer()
+    new Metrics().initialize(camp)
 
     const res = await fetch(`${baseUrl}/metrics`)
 
@@ -45,10 +44,10 @@ describe('Prometheus metrics route', function() {
   })
 
   it('returns metrics for allowed IP', async function() {
-    startServer({
+    new Metrics({
       enabled: true,
       allowedIps: '^(127\\.0\\.0\\.1|::1|::ffff:127\\.0\\.0\\.1)$',
-    })
+    }).initialize(camp)
 
     const res = await fetch(`${baseUrl}/metrics`)
 
@@ -57,10 +56,10 @@ describe('Prometheus metrics route', function() {
   })
 
   it('returns metrics for request from allowed remote address', async function() {
-    startServer({
+    new Metrics({
       enabled: true,
       allowedIps: '^(127\\.0\\.0\\.1|::1|::ffff:127\\.0\\.0\\.1)$',
-    })
+    }).initialize(camp)
 
     const res = await fetch(`${baseUrl}/metrics`)
 
@@ -69,10 +68,10 @@ describe('Prometheus metrics route', function() {
   })
 
   it('returns 403 for not allowed IP', async function() {
-    startServer({
+    new Metrics({
       enabled: true,
       allowedIps: '^127\\.0\\.0\\.200$',
-    })
+    }).initialize(camp)
 
     const res = await fetch(`${baseUrl}/metrics`)
 
@@ -81,9 +80,9 @@ describe('Prometheus metrics route', function() {
   })
 
   it('returns 403 for every request when list with allowed IPs not defined', async function() {
-    startServer({
+    new Metrics({
       enabled: true,
-    })
+    }).initialize(camp)
 
     const res = await fetch(`${baseUrl}/metrics`)
 

lib/sys/rate-limit.js

@@ -11,7 +11,12 @@ module.exports = class RateLimit {
     this.banned = new Set()
     this.bannedUrls = new Set()
     this.whitelist = options.whitelist || /(?!)/ // Matches nothing by default.
-    setInterval(this.resetHits.bind(this), this.period * 1000)
+    this.interval = setInterval(this.resetHits.bind(this), this.period * 1000)
+  }
+
+  stop() {
+    clearInterval(this.interval)
+    this.interval = undefined
   }
 
   resetHits() {