Stop violating types by returning assertions from Mocha tests. (#972)

This change was introduced by the changes in apollographql/apollo-server#802
but first showed its head in apollographql/apollo-server#908.  The reason that
violations in new type definitions aren't being found until subsequent PRs
isn't entirely clear but, ignoring that CI-related annoyance, the problem
itself here is very concrete.

It traces back to a major version update to `@types/mocha` via [Exhibit A],
which makes it unacceptable to return anything besides a `Promise` or
_nothing_ from a Mocha test factory.

I agree with this change in principle, since generally speaking there can be
multiple `expect` statements in each test and there is no particular reason
that the last one's value should be getting returned as Mocha doesn't do
anything functional with it.

More than anything, this seems like an artifact of an ESLint rule which
mandated that the last value in a function be returned, à la CoffeeScript or
other languages.

This will fix the failing tests on apollographql/apollo-server#908 and other
PRs currently in-flight.

Exhibit A: https://github.com/DefinitelyTyped/DefinitelyTyped/pull/24301

packages/apollo-server-core/src/runQuery.test.ts

@@ -92,7 +92,7 @@ describe('runQuery', () => {
     const query = `{ testString }`;
     const expected = { testString: 'it works' };
     return runQuery({ schema, query: query }).then(res => {
-      return expect(res.data).to.deep.equal(expected);
+      expect(res.data).to.deep.equal(expected);
     });
   });
 
@@ -100,7 +100,7 @@ describe('runQuery', () => {
     const query = parse(`{ testString }`);
     const expected = { testString: 'it works' };
     return runQuery({ schema, query: query }).then(res => {
-      return expect(res.data).to.deep.equal(expected);
+      expect(res.data).to.deep.equal(expected);
     });
   });
 
@@ -114,7 +114,7 @@ describe('runQuery', () => {
     }).then(res => {
       expect(res.data).to.be.undefined;
       expect(res.errors.length).to.equal(1);
-      return expect(res.errors[0].message).to.match(expected);
+      expect(res.errors[0].message).to.match(expected);
     });
   });
 
@@ -129,7 +129,7 @@ describe('runQuery', () => {
     }).then(res => {
       logStub.restore();
       expect(logStub.callCount).to.equal(1);
-      return expect(logStub.getCall(0).args[0]).to.match(expected);
+      expect(logStub.getCall(0).args[0]).to.match(expected);
     });
   });
 
@@ -142,7 +142,7 @@ describe('runQuery', () => {
       debug: false,
     }).then(res => {
       logStub.restore();
-      return expect(logStub.callCount).to.equal(0);
+      expect(logStub.callCount).to.equal(0);
     });
   });
 
@@ -157,7 +157,7 @@ describe('runQuery', () => {
     }).then(res => {
       expect(res.data).to.be.undefined;
       expect(res.errors.length).to.equal(1);
-      return expect(res.errors[0].message).to.deep.equal(expected);
+      expect(res.errors[0].message).to.deep.equal(expected);
     });
   });
 
@@ -166,7 +166,7 @@ describe('runQuery', () => {
     const expected = { testRootValue: 'it also works' };
     return runQuery({ schema, query: query, rootValue: 'it also' }).then(
       res => {
-        return expect(res.data).to.deep.equal(expected);
+        expect(res.data).to.deep.equal(expected);
       },
     );
   });
@@ -175,7 +175,7 @@ describe('runQuery', () => {
     const query = `{ testContextValue }`;
     const expected = { testContextValue: 'it still works' };
     return runQuery({ schema, query: query, context: 'it still' }).then(res => {
-      return expect(res.data).to.deep.equal(expected);
+      expect(res.data).to.deep.equal(expected);
     });
   });
 
@@ -192,7 +192,7 @@ describe('runQuery', () => {
       },
     }).then(res => {
       expect(res.data).to.deep.equal(expected);
-      return expect(res['extensions']).to.equal('it still');
+      expect(res['extensions']).to.equal('it still');
     });
   });
 
@@ -204,7 +204,7 @@ describe('runQuery', () => {
       query: query,
       variables: { base: 1 },
     }).then(res => {
-      return expect(res.data).to.deep.equal(expected);
+      expect(res.data).to.deep.equal(expected);
     });
   });
 
@@ -216,7 +216,7 @@ describe('runQuery', () => {
       schema,
       query: query,
     }).then(res => {
-      return expect(res.errors[0].message).to.deep.equal(expected);
+      expect(res.errors[0].message).to.deep.equal(expected);
     });
   });
 
@@ -243,7 +243,7 @@ describe('runQuery', () => {
       testString: 'it works',
     };
     return runQuery({ schema, query: query, operationName: 'Q1' }).then(res => {
-      return expect(res.data).to.deep.equal(expected);
+      expect(res.data).to.deep.equal(expected);
     });
   });
 

packages/apollo-server-express/src/apolloServerHttp.test.ts

@@ -492,7 +492,7 @@ describe(`GraphQL-HTTP (apolloServer) tests for ${version} express`, () => {
 
       expect(response.status).to.equal(405);
       expect(response.headers.allow).to.equal('GET, POST');
-      return expect(response.text).to.contain(
+      expect(response.text).to.contain(
         'Apollo Server supports only GET/POST requests.',
       );
     });

packages/apollo-server-integration-testsuite/src/index.ts

@@ -170,7 +170,7 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
           });
         return req.then(res => {
           expect(res.status).to.equal(200);
-          return expect(res.body.data).to.deep.equal(expected);
+          expect(res.body.data).to.deep.equal(expected);
         });
       });
 
@@ -192,7 +192,7 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
           });
         return req.then(res => {
           expect(res.status).to.equal(200);
-          return expect(res.body.data).to.deep.equal(expected);
+          expect(res.body.data).to.deep.equal(expected);
         });
       });
 
@@ -210,7 +210,7 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
           });
         return req.then(res => {
           expect(res.status).to.equal(500);
-          return expect(res.error.text).to.contain(expected);
+          expect(res.error.text).to.contain(expected);
         });
       });
 
@@ -232,7 +232,7 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
           .send();
         return req.then(res => {
           expect(res.status).to.equal(500);
-          return expect(res.error.text).to.contain('POST body missing.');
+          expect(res.error.text).to.contain('POST body missing.');
         });
       });
 
@@ -241,7 +241,7 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
         const req = request(app).get(`/graphql`);
         return req.then(res => {
           expect(res.status).to.equal(400);
-          return expect(res.error.text).to.contain('GET query missing.');
+          expect(res.error.text).to.contain('GET query missing.');
         });
       });
 
@@ -258,7 +258,7 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
           .query(query);
         return req.then(res => {
           expect(res.status).to.equal(200);
-          return expect(res.body.data).to.deep.equal(expected);
+          expect(res.body.data).to.deep.equal(expected);
         });
       });
 
@@ -275,7 +275,7 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
           .query(query);
         return req.then(res => {
           expect(res.status).to.equal(200);
-          return expect(res.body.data).to.deep.equal(expected);
+          expect(res.body.data).to.deep.equal(expected);
         });
       });
 
@@ -290,7 +290,7 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
         return req.then(res => {
           expect(res.status).to.equal(405);
           expect(res.headers['allow']).to.equal('POST');
-          return expect(res.error.text).to.contain(
+          expect(res.error.text).to.contain(
             'GET supports only query operation',
           );
         });
@@ -315,7 +315,7 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
         return req.then(res => {
           expect(res.status).to.equal(405);
           expect(res.headers['allow']).to.equal('POST');
-          return expect(res.error.text).to.contain(
+          expect(res.error.text).to.contain(
             'GET supports only query operation',
           );
         });
@@ -335,7 +335,7 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
           .query(query);
         return req.then(res => {
           expect(res.status).to.equal(200);
-          return expect(res.body.data).to.deep.equal(expected);
+          expect(res.body.data).to.deep.equal(expected);
         });
       });
 
@@ -351,7 +351,7 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
           });
         return req.then(res => {
           expect(res.status).to.equal(200);
-          return expect(res.body.data).to.deep.equal(expected);
+          expect(res.body.data).to.deep.equal(expected);
         });
       });
 
@@ -370,7 +370,7 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
         return req.then(res => {
           expect(res.status).to.equal(200);
           expect(res.body.data).to.deep.equal(expected);
-          return expect(res.body.extensions).to.deep.equal({
+          expect(res.body.extensions).to.deep.equal({
             cacheControl: {
               version: 1,
               hints: [{ maxAge: 0, path: ['testPerson'] }],
@@ -394,7 +394,7 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
         return req.then(res => {
           expect(res.status).to.equal(200);
           expect(res.body.data).to.deep.equal(expected);
-          return expect(res.body.extensions).to.deep.equal({
+          expect(res.body.extensions).to.deep.equal({
             cacheControl: {
               version: 1,
               hints: [{ maxAge: 5, path: ['testPerson'] }],
@@ -416,7 +416,7 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
           });
         return req.then(res => {
           expect(res.status).to.equal(200);
-          return expect(res.body.data).to.deep.equal(expected);
+          expect(res.body.data).to.deep.equal(expected);
         });
       });
 
@@ -433,7 +433,7 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
           });
         return req.then(res => {
           expect(res.status).to.equal(200);
-          return expect(res.body.data).to.deep.equal(expected);
+          expect(res.body.data).to.deep.equal(expected);
         });
       });
 
@@ -447,9 +447,7 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
           });
         return req.then(res => {
           expect(res.status).to.equal(400);
-          return expect(res.error.text).to.contain(
-            'Variables are invalid JSON.',
-          );
+          expect(res.error.text).to.contain('Variables are invalid JSON.');
         });
       });
 
@@ -469,7 +467,7 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
           });
         return req.then(res => {
           expect(res.status).to.equal(200);
-          return expect(res.body.data).to.deep.equal(expected);
+          expect(res.body.data).to.deep.equal(expected);
         });
       });
 
@@ -480,9 +478,9 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
           .send({ query: introspectionQuery });
         return req.then(res => {
           expect(res.status).to.equal(200);
-          return expect(
-            res.body.data.__schema.types[0].fields[0].name,
-          ).to.equal('testString');
+          expect(res.body.data.__schema.types[0].fields[0].name).to.equal(
+            'testString',
+          );
         });
       });
 
@@ -519,7 +517,7 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
           ]);
         return req.then(res => {
           expect(res.status).to.equal(200);
-          return expect(res.body).to.deep.equal(expected);
+          expect(res.body).to.deep.equal(expected);
         });
       });
 
@@ -545,7 +543,7 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
           ]);
         return req.then(res => {
           expect(res.status).to.equal(200);
-          return expect(res.body).to.deep.equal(expected);
+          expect(res.body).to.deep.equal(expected);
         });
       });
 
@@ -570,7 +568,7 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
           );
         return req.then(res => {
           expect(res.status).to.equal(200);
-          return expect(res.body).to.deep.equal(expected);
+          expect(res.body).to.deep.equal(expected);
         });
       });
 
@@ -605,7 +603,7 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
           ]);
         return req.then(res => {
           expect(res.status).to.equal(200);
-          return expect(res.body).to.deep.equal(expected);
+          expect(res.body).to.deep.equal(expected);
         });
       });
 
@@ -645,7 +643,7 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
         return req.then(res => {
           expect(callCount).to.equal(2);
           expect(res.status).to.equal(200);
-          return expect(res.body).to.deep.equal(expected);
+          expect(res.body).to.deep.equal(expected);
         });
       });
 
@@ -662,7 +660,7 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
           });
         return req.then(res => {
           expect(res.status).to.equal(200);
-          return expect(res.body.data).to.deep.equal(expected);
+          expect(res.body.data).to.deep.equal(expected);
         });
       });
 
@@ -685,7 +683,7 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
           });
         return req.then(res => {
           expect(res.status).to.equal(200);
-          return expect(res.body.extensions).to.deep.equal(expected);
+          expect(res.body.extensions).to.deep.equal(expected);
         });
       });
 
@@ -704,7 +702,7 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
           });
         return req.then(res => {
           expect(res.status).to.equal(200);
-          return expect(res.body.data.testContext).to.equal(expected);
+          expect(res.body.data.testContext).to.equal(expected);
         });
       });
 
@@ -723,7 +721,7 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
           });
         return req.then(res => {
           expect(res.status).to.equal(200);
-          return expect(res.body.data.testRootValue).to.equal(expected);
+          expect(res.body.data.testRootValue).to.equal(expected);
         });
       });
 
@@ -741,7 +739,7 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
           });
         return req.then(res => {
           expect(res.status).to.equal(200);
-          return expect(res.body.errors[0].message).to.equal(expected);
+          expect(res.body.errors[0].message).to.equal(expected);
         });
       });
 
@@ -760,7 +758,7 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
           });
         return req.then(res => {
           expect(res.status).to.equal(200);
-          return expect(res.body.errors[0].message).to.equal(expected);
+          expect(res.body.errors[0].message).to.equal(expected);
         });
       });
 
@@ -779,9 +777,7 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
             query: 'query test{ testError }',
           });
         return req.then(res => {
-          return expect(res.body.errors[0].message).to.equal(
-            'Internal server error',
-          );
+          expect(res.body.errors[0].message).to.equal('Internal server error');
         });
       });
 
@@ -803,7 +799,7 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
           });
         return req.then(res => {
           console.error = origError;
-          return expect(stackTrace[0][0]).to.match(expected);
+          expect(stackTrace[0][0]).to.match(expected);
         });
       });
 
@@ -824,7 +820,7 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
         return req.then(res => {
           logStub.restore();
           expect(logStub.callCount).to.equal(1);
-          return expect(logStub.getCall(0).args[0]).to.match(expected);
+          expect(logStub.getCall(0).args[0]).to.match(expected);
         });
       });
 
@@ -851,7 +847,7 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
           });
         return req.then(res => {
           expect(res.status).to.equal(400);
-          return expect(res.body.errors[0].message).to.equal(expected);
+          expect(res.body.errors[0].message).to.equal(expected);
         });
       });
     });
@@ -967,7 +963,7 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
           });
         return req.then(res => {
           expect(res.status).to.equal(200);
-          return expect(res.body.data).to.deep.equal(expected);
+          expect(res.body.data).to.deep.equal(expected);
         });
       });
 
@@ -1013,7 +1009,7 @@ export default (createApp: CreateAppFunc, destroyApp?: DestroyAppFunc) => {
           ]);
         return req.then(res => {
           expect(res.status).to.equal(200);
-          return expect(res.body).to.deep.equal(expected);
+          expect(res.body).to.deep.equal(expected);
         });
       });
     });

packages/apollo-server-module-operation-store/src/operationStore.test.ts

@@ -59,7 +59,7 @@ describe('operationStore', () => {
     const store = new OperationStore(schema);
     store.put(query);
 
-    return expect(print(store.get('testquery'))).to.deep.equal(expected);
+    expect(print(store.get('testquery'))).to.deep.equal(expected);
   });
 
   it('can store a Document and return its ast', () => {
@@ -69,7 +69,7 @@ describe('operationStore', () => {
     const store = new OperationStore(schema);
     store.put(parse(query));
 
-    return expect(print(store.get('testquery'))).to.deep.equal(expected);
+    expect(print(store.get('testquery'))).to.deep.equal(expected);
   });
 
   it('can store queries and return them with getMap', () => {
@@ -79,21 +79,21 @@ describe('operationStore', () => {
     const store = new OperationStore(schema);
     store.put(query);
     store.put(query2);
-    return expect(store.getMap().size).to.equal(2);
+    expect(store.getMap().size).to.equal(2);
   });
 
   it('throws a parse error if the query is invalid', () => {
     const query = `query testquery{ testString`;
 
     const store = new OperationStore(schema);
-    return expect(() => store.put(query)).to.throw(/Syntax Error/);
+    expect(() => store.put(query)).to.throw(/Syntax Error/);
   });
 
   it('throws a validation error if the query is invalid', () => {
     const query = `query testquery { testStrin }`;
 
     const store = new OperationStore(schema);
-    return expect(() => store.put(query)).to.throw(/Cannot query field/);
+    expect(() => store.put(query)).to.throw(/Cannot query field/);
   });
 
   it('throws an error if there is more than one query or mutation', () => {
@@ -103,7 +103,7 @@ describe('operationStore', () => {
       `;
 
     const store = new OperationStore(schema);
-    return expect(() => store.put(query)).to.throw(
+    expect(() => store.put(query)).to.throw(
       /OperationDefinitionNode must contain only one definition/,
     );
   });
@@ -117,7 +117,7 @@ describe('operationStore', () => {
 
     const store = new OperationStore(schema);
 
-    return expect(() => store.put(query)).to.throw(/must contain at least/);
+    expect(() => store.put(query)).to.throw(/must contain at least/);
   });
 
   it('can delete stored operations', () => {
@@ -127,6 +127,6 @@ describe('operationStore', () => {
     store.put(query);
     store.delete('testquery');
 
-    return expect(store.get('testquery')).to.be.undefined;
+    expect(store.get('testquery')).to.be.undefined;
   });
 });