bug(auth): Fix test timeouts

Becuase:
- After upgrading sentry we were seeing test timeouts
- There were some kinda weird patterns in place for these tests anyways
- Test server wasn't getting cleaned up
- Test server start up is taking longer (presumably because of sentry upgrade?)

This Commit:
- Moves before an after blocks next to each other so setup and tear down are easy to follow
- Makes sure test server start / stop is done in before each blocks
- Increase test timeouts to 60s when test server is used
- Switches to async/await for before and after blocks, which feels less error prone

packages/fxa-auth-server/bin/key_server.js

@@ -287,15 +287,39 @@ async function run(config) {
     log: log,
     async close() {
       log.info('shutdown');
-      await server.stop();
-      statsd.close();
+
+      try {
+        await server.stop();
+      } catch (e) {
+        log.warn('shutdown', {
+          message: 'Server did not shut down cleanly. ' + e.message,
+        });
+      }
+
+      try {
+        statsd.close();
+      } catch (e) {
+        log.warn('shutdown', {
+          message: 'Statsd did not shut down cleanly. ' + e.message,
+        });
+      }
+
       try {
         senders.email.stop();
       } catch (e) {
         // XXX: simplesmtp module may quit early and set socket to `false`, stopping it may fail
-        log.warn('shutdown', { message: 'Mailer client already disconnected' });
+        log.warn('shutdown', {
+          message: 'senders.email did not shut down cleanly. ' + e.message
+        });
+      }
+
+      try {
+        await database.close();
+      } catch (e) {
+        log.warn('shutdown', {
+          message: 'Database connection did not shutdown cleanly. ' + e.message,
+        });
       }
-      await database.close();
     },
   };
 }

packages/fxa-auth-server/scripts/test-remote-quick.js

@@ -18,7 +18,7 @@ TestServer.start(config, false).then((server) => {
     { stdio: 'inherit' }
   );
 
-  cp.on('close', (code) => {
-    server.stop();
+  cp.on('close', async (code) => {
+    await server.stop();
   });
 });

packages/fxa-auth-server/test/client/index.js

@@ -95,26 +95,23 @@ module.exports = (config) => {
     this.unwrapBKeyVersion2 = unwrapBKeyVersion2;
   };
 
-  Client.create = function (origin, email, password, options = {}) {
+  Client.create = async function (origin, email, password, options = {}) {
     const c = new Client(origin, options);
 
     if (options.version === 'V2') {
-      return (async function () {
-        await c.setupCredentials(email, password);
-        await c.setupCredentialsV2(email, password);
+      await c.setupCredentials(email, password);
+      await c.setupCredentialsV2(email, password);
 
-        c.generateNewWrapKb();
-        c.deriveWrapKbVersion2FromKb();
+      c.generateNewWrapKb();
+      c.deriveWrapKbVersion2FromKb();
 
-        await c.createV2();
-
-        return c;
-      })();
+      const result = await c.createV2();
+      return result;
+    } else {
+      await c.setupCredentials(email, password);
+      const result = await c.create(options);
+      return result;
     }
-
-    return c.setupCredentials(email, password).then(() => {
-      return c.create(options);
-    });
   };
 
   Client.login = function (origin, email, password, options) {

packages/fxa-auth-server/test/oauth/api.js

@@ -16,8 +16,6 @@ const sinon = require('sinon');
 const db = require('../../lib/oauth/db');
 const encrypt = require('fxa-shared/auth/encrypt');
 const config = testServer.config;
-let Server;
-let Server2;
 
 const unique = require('../../lib/oauth/unique');
 const util = require('../../lib/oauth/util');
@@ -128,35 +126,6 @@ function authParams(params, options) {
   return defaults;
 }
 
-function newToken(payload = {}, options = {}) {
-  var ttl = payload.ttl || MAX_TTL_S;
-  delete payload.ttl;
-  mockAssertion().reply(200, options.verifierResponse || VERIFY_GOOD);
-  return Server.api
-    .post({
-      url: '/authorization',
-      payload: authParams(payload, options),
-    })
-    .then(function (res) {
-      assert.equal(res.statusCode, 200);
-      assertSecurityHeaders(res);
-      return Server.api.post({
-        url: '/token',
-        payload: {
-          client_id: options.clientId || clientId,
-          client_secret: options.codeVerifier
-            ? undefined
-            : options.secret || secret,
-          code: res.result.code,
-          code_verifier: options.codeVerifier,
-          ppid_seed: options.ppidSeed,
-          resource: options.resource,
-          ttl: ttl,
-        },
-      });
-    });
-}
-
 function assertInvalidRequestParam(result, param) {
   assert.equal(result.code, 400);
   assert.equal(result.errno, 109);
@@ -187,36 +156,65 @@ function basicAuthHeader(clientId, secret) {
 }
 
 describe('#integration - /v1', function () {
+  this.timeout(60000);
   const VERIFY_FAILURE = '{"status": "failure"}';
   let sandbox;
+  let Server;
+
+  function newToken(payload = {}, options = {}) {
+    var ttl = payload.ttl || MAX_TTL_S;
+    delete payload.ttl;
+    mockAssertion().reply(200, options.verifierResponse || VERIFY_GOOD);
+    return Server.api
+      .post({
+        url: '/authorization',
+        payload: authParams(payload, options),
+      })
+      .then(function (res) {
+        assert.equal(res.statusCode, 200);
+        assertSecurityHeaders(res);
+        return Server.api.post({
+          url: '/token',
+          payload: {
+            client_id: options.clientId || clientId,
+            client_secret: options.codeVerifier
+              ? undefined
+              : options.secret || secret,
+            code: res.result.code,
+            code_verifier: options.codeVerifier,
+            ppid_seed: options.ppidSeed,
+            resource: options.resource,
+            ttl: ttl,
+          },
+        });
+      });
+  }
 
   before(async function () {
-    this.timeout(30000);
+    const start = Date.now();
+    console.log('!!! 1', Date.now() - start);
     Server = await testServer.start();
-    return Promise.all([
-      genAssertion(USERID + config.get('oauthServer.browserid.issuer')).then(
-        function (ass) {
-          AN_ASSERTION = ass;
-        }
-      ),
-      db.ping().then(function () {
-        client = clientByName('Mocha');
-        clientId = client.id;
-        assert.equal(encrypt.hash(secret).toString('hex'), client.hashedSecret);
-        assert.equal(
-          encrypt.hash(secretPrevious).toString('hex'),
-          client.hashedSecretPrevious
-        );
-        badSecret = Buffer.from(secret, 'hex').slice();
-        badSecret[badSecret.length - 1] ^= 1;
-        badSecret = badSecret.toString('hex');
-      }),
-    ]);
+    console.log('!!! 2', Date.now() - start);
+    AN_ASSERTION = await genAssertion(
+      USERID + config.get('oauthServer.browserid.issuer')
+    );
+    await db.ping();
+    console.log('!!! 3', Date.now() - start);
+    client = clientByName('Mocha');
+    clientId = client.id;
+    assert.equal(encrypt.hash(secret).toString('hex'), client.hashedSecret);
+    assert.equal(
+      encrypt.hash(secretPrevious).toString('hex'),
+      client.hashedSecretPrevious
+    );
+    badSecret = Buffer.from(secret, 'hex').slice();
+    badSecret[badSecret.length - 1] ^= 1;
+    badSecret = badSecret.toString('hex');
+    console.log('!!! 4', Date.now() - start);
   });
 
   after(async function () {
-    await Server?.close();
-    await Server2?.close();
+    await Server.close();
   });
 
   beforeEach(() => {
@@ -2860,37 +2858,57 @@ describe('#integration - /v1', function () {
       assert.equal(res.result.errno, 115);
     });
 
-    it('should not reject expired tokens from pocket clients', async function () {
+    describe('expired tokens from pocket clients', () => {
       const clientId = '749818d3f2e7857f';
-      config.set('oauthServer.expiration.accessTokenExpiryEpoch', undefined);
-      Server2 = await testServer.start();
-      let res = await newToken(
-        {
-          ttl: 1,
-        },
-        {
-          clientId,
-        }
-      );
+      let accessTokenExpiryEpoch;
 
-      assert.equal(res.statusCode, 200);
-      assertSecurityHeaders(res);
-      assert.equal(res.result.expires_in, 1);
-
-      sandbox.useFakeTimers({
-        now: Date.now() + 1000 * 60 * 60, // 1 hr in future
-        shouldAdvanceTime: true,
+      before(async () => {
+        await Server.close();
+        accessTokenExpiryEpoch = config.get(
+          'oauthServer.expiration.accessTokenExpiryEpoch'
+        );
+        config.set('oauthServer.expiration.accessTokenExpiryEpoch', undefined);
+        Server = await testServer.start();
       });
 
-      res = await Server2.api.post({
-        url: '/verify',
-        payload: {
-          token: res.result.access_token,
-        },
+      after(async () => {
+        await Server.close();
+        config.set(
+          'oauthServer.expiration.accessTokenExpiryEpoch',
+          accessTokenExpiryEpoch
+        );
+        Server = await testServer.start();
       });
 
-      assert.equal(res.statusCode, 200);
-      assertSecurityHeaders(res);
+      it('should not reject expired tokens from pocket clients', async function () {
+        let res = await newToken(
+          {
+            ttl: 1,
+          },
+          {
+            clientId,
+          }
+        );
+
+        assert.equal(res.statusCode, 200);
+        assertSecurityHeaders(res);
+        assert.equal(res.result.expires_in, 1);
+
+        sandbox.useFakeTimers({
+          now: Date.now() + 1000 * 60 * 60, // 1 hr in future
+          shouldAdvanceTime: true,
+        });
+
+        res = await Server.api.post({
+          url: '/verify',
+          payload: {
+            token: res.result.access_token,
+          },
+        });
+
+        assert.equal(res.statusCode, 200);
+        assertSecurityHeaders(res);
+      });
     });
 
     describe('response', function () {

packages/fxa-auth-server/test/oauth/key-management-scripts.js

@@ -11,11 +11,10 @@ const crypto = require('crypto');
 const rimraf = require('rimraf');
 
 describe('#integration - the signing-key management scripts', function () {
+  this.timeout(60000);
   let runScript;
   let workDir, keyFile, newKeyFile, oldKeyFile;
 
-  this.timeout(30000);
-
   beforeEach(() => {
     const uniqName = crypto.randomBytes(8).toString('hex');
     workDir = path.join(os.tmpdir(), `fxa-oauth-server-tests-${uniqName}`);

packages/fxa-auth-server/test/remote/account_create_tests.js

@@ -21,9 +21,10 @@ const {
 // Note, intentionally not indenting for code review.
 [{ version: '' }, { version: 'V2' }].forEach((testOptions) => {
   describe(`#integration${testOptions.version} - remote account create`, function () {
-    this.timeout(50000);
+    this.timeout(60000);
     let server;
-    before(async () => {
+
+    before(async function () {
       config.subscriptions = {
         enabled: true,
         stripeApiKey: 'fake_key',
@@ -54,6 +55,10 @@ const {
       return server;
     });
 
+    after(async function () {
+      await TestServer.stop(server);
+    });
+
     it('unverified account fail when getting keys', () => {
       const email = server.uniqueEmail();
       const password = 'allyourbasearebelongtous';
@@ -989,9 +994,7 @@ const {
       assert.equal(kB2, originalKb);
     });
 
-    after(async () => {
-      await TestServer.stop(server);
-    });
+
 
     async function login(email, password, version = '') {
       return await Client.login(config.publicUrl, email, password, {

packages/fxa-auth-server/test/remote/account_create_with_code_tests.js

@@ -15,13 +15,16 @@ const otplib = require('otplib');
 [{version:""},{version:"V2"}].forEach((testOptions) => {
 
 describe(`#integration${testOptions.version} - remote account create with sign-up code`, function () {
-  this.timeout(15000);
+  this.timeout(60000);
   const password = '4L6prUdlLNfxGIoj';
   let server, client, email, emailStatus, emailData;
 
   before(async () => {
     server = await TestServer.start(config);
-    return server;
+  });
+
+  after(async function () {
+    await TestServer.stop(server);
   });
 
   it('create and verify sync account', async () => {
@@ -231,9 +234,7 @@ describe(`#integration${testOptions.version} - remote account create with sign-u
     });
   });
 
-  after(() => {
-    return TestServer.stop(server);
-  });
+
 });
 
 });