Testing infrastructure improvements #453

martijnwalraven · 2021-02-04T14:56:10Z

TODO

Revert dfb2c71 after this lands. (note added by @abernix) (done with b6e3bad)

This is a ~~draft~~ PR with improvements to the JavaScript testing infrastructure.

Update Jest and related packages to latest versions, update TypeScript to latest version

TypeScript 4.0 introduced support for noEmit with composite projects, which is needed to make our test project configurations valid.

Ensure are all our test files are type checked

We weren't actually type checking our testing code. I've tried to fix all TypeScript and Jest configurations to make sure we do, and we do so consistently. This doesn't affect running the tests, because we set diagnostics to false for ts-jest. But these errors are still logged. I'd like to fix them before merging this PR, but that is WIP.

Move snapshot serializers and matchers to `federation-integration-testsuite`

Snapshot serializers were duplicated in @apollo/federation and @apollo/gateway. This moves both the snapshot serializers and the matchers to apollo-federaton-integration-testsuite (we may want to rename that package).

I temporarily disabled the option for the gateway to include the pretty printed query plan under __queryPlanExperimental (which is used to show the query plan in the Playground we ship with Apollo Server), because this lead to a circular dependency between @apollo/gateway and apollo-federaton-integration-testsuite`. We should either solve that or switch Playground to use the JSON serialization format for the query plan.

Fix Jest snapshot indentation for `graphql-js` AST

We need to re-indent the lines printed from graphql-js in astSerializer, because Jest has started to ignore indentation when diffing snapshots, and it does this by invoking snapshot serializers with these values set to 0.

Without re-indenting, every line printed from graphql-js would be shown as changed. See jestjs/jest#9203.

Remove use of `moduleNameMapper` for local imports

In some of our other repositories, we use Jest's moduleNameMapper option to import files from their src instead of their dist directory. That means we rely on ts-jest to compile not just the package currently under test, but also any imported code from other packages in the same repository.

The main benefit of that was that we didn't have to compile all packages before each test run. Jest would only compile what was imported, and only when the file had changed (because Jest includes its own file cache). That did sometimes lead to issues however, because the file cache doesn't correctly keep track of dependencies. ts-jest was also often unable to apply the right tsconfig.json to imported projects.

Nowadays, tsc's support for incremental compilation means that whole project compiles are really fast. Relying on tsc to compile our code is often even faster than the per file compilation that ts-jest is forced to perform. And it means dependencies are tracked correctly and the right configs are applied. The project is compiled before each run of npm test through a pretest script, so the code your tests import should never be out of date.

Previously, we used Jest's `moduleNameMapper` option to import files from their `src` instead of their `dist` directory. That means we rely on `ts-jest` to compile not just the package currently under test, but also any imported code from other packages in the same repository. The main benefit of that was that we didn't have to compile all packages before each test run. Jest would only compile what was imported, and only when the file had changed (because Jest includes its own file cache). That did sometimes lead to issues however, because the file cache doesn't correctly keep track of dependencies. `ts-jest` was also often unable to apply the right `tsconfig.json` to imported projects. Nowadays, `tsc`'s support for incremental compilation means that whole project compiles are really fast. Relying on `tsc` to compile our code is often even faster than the per file compilation that `ts-jest` is forced to perform. And it means dependencies are tracked correctly and the right configs are applied. The project is compiled before each run of `npm test` through a `pretest` script, so the code your tests import should never be out of date.

TypeScript 4.0 introduced support for `noEmit` with composite projects, which is needed to make our test project configurations valid.

…t.json This allows our root `tsconfig.test.json` to indirectly reference all test files, which makes it possible for `tsc` to compile them all at the same time (or rather type check, because we use `noEmit`). Unfortunately, we still need an additional `tsconfig.json` per `__tests__` directory to satisfy VS Code due to microsoft/vscode/#12463.

Running this task watches both regular source files and test files, and incrementally compiles any changed files to show errors and warnings in the problem panel.

… coalescing Having a `__tests__` directory specific override leads to a discrepancy with the package-wide `tsconfig.test.json`. Fortunately, the addition of null coalescing makes it a lot easier to avoid these errors.

…an manually This seems to have been a contributor PR that slipped through, because we use `queryPlanSerializer` everywhere else.

…uite Snapshot serializers were duplicated in `@apollo/federation` and `@apollo/gateway`. This moves both the snapshot serializers and the matchers to `apollo-federaton-integration-testsuite` (we may want to rename that package). I temporarily disabled the option for the gateway to include the pretty printed query plan under `__queryPlanExperimental` (which is used to show the query plan in the Playground we ship with Apollo Server), because this lead to a circular dependency between `@apollo/gateway` and apollo-federaton-integration-testsuite`. We should either solve that or switch Playground to use the JSON serialization format for the query plan.

We need to re-indent the lines printed from `graphql-js` in `astSerializer`, because Jest has started to ignore indentation when diffing snapshots, and it does this by invoking snapshot serializers with these values set to 0. Without re-indenting, every line printed from `graphql-js` would be shown as changed. See jestjs/jest#9203.

It turns out the `astSerializer` in `@apollo/gateway` (not the one in `@apollo/federation`) reduced double newlines to single ones. Since we no longer do this (and can't because that would break other tests), correcting the snapshots to account for that.

Our peer dependencies still support v14, but this ensures we test against the latest version and also avoid a typing error because `specifiedByUrl` was only added in v15.

Unfortunately, we need an additional `tsconfig.json` per `__tests__` directory to satisfy VS Code due to microsoft/vscode/#12463. I tried having these extend the `tsconfig.test.json` in their respective packages, but it turns out `references` are not inherited (see microsoft/TypeScript#27098).

…test `tsconfig.json`

trevor-scheer

🎉

trevor-scheer · 2021-02-09T21:10:50Z

package.json

@@ -11,6 +11,7 @@
    "release:version-bump": "lerna version --force-publish=@apollo/query-planner-wasm",
    "release:start-ci-publish": "node -p '`Publish (dist-tag:${process.env.APOLLO_DIST_TAG || \"latest\"})`' | git tag -F - \"publish/$(date -u '+%Y%m%d%H%M%S')\" && git push origin \"$(git describe --match='publish/*' --tags --exact-match HEAD)\"",
    "postinstall": "npm run rustup-ensure && lerna run monorepo-prepare --stream && npm run compile",
+    "pretest": "npm run compile",


Your commit message reminds me - the cached TS builds were problematic for us when they were first introduced. Maybe we can stop wiping that cache in our clean script?

While I agree with it possibly being more reliable and stable than it was when it was first released, I think I would still prefer — sanity-wise — if clean really did a proper clean, including cached build artifacts.

gateway-js/src/__tests__/testSetup.ts

gateway-js/src/index.ts

federation-js/src/__tests__/testSetup.ts

glasser · 2021-02-09T21:53:02Z

I volunteered to review this more for my own education than because I feel the need to personally sign up, so while I will eventually take a look, don't block on me!

abernix

Thanks so much for these improvements. I have one change requesting that we avoid a top-level module export * from './QueryPlan' if at all possible (noted within my reivew), but I won't block on it not being addressed/addressable.

abernix · 2021-02-22T14:27:57Z

package.json

@@ -11,6 +11,7 @@
    "release:version-bump": "lerna version --force-publish=@apollo/query-planner-wasm",
    "release:start-ci-publish": "node -p '`Publish (dist-tag:${process.env.APOLLO_DIST_TAG || \"latest\"})`' | git tag -F - \"publish/$(date -u '+%Y%m%d%H%M%S')\" && git push origin \"$(git describe --match='publish/*' --tags --exact-match HEAD)\"",
    "postinstall": "npm run rustup-ensure && lerna run monorepo-prepare --stream && npm run compile",
+    "pretest": "npm run compile",


While I agree with it possibly being more reliable and stable than it was when it was first released, I think I would still prefer — sanity-wise — if clean really did a proper clean, including cached build artifacts.

abernix · 2021-02-22T14:31:05Z

federation-integration-testsuite-js/src/matchers/toHaveFetched.ts

@@ -15,8 +16,8 @@ function prepareHttpOptions(requestUrl: string, requestOpts: RequestInit): Reque
  const headers = new Headers();
  headers.set('Content-Type', 'application/json');
  if (requestOpts.headers) {
-    for (let name in requestOpts.headers) {
-      headers.set(name, requestOpts.headers[name]);
+    for (const [name, value] of new Headers(requestOpts.headers)) {


Just so I'm understanding, this is necessary — rather than just headers.entries() to account for the fact that headers might be a { [name: string]: string }?

This code hasn't changed in this PR, but I'm wondering why we can't directly initialize headers as new Headers(requestOpts.headers). What is iterating over the passed in headers and adding them one by one adding?

Also, the types of toHaveFetched are currently broken because the normal BodyInit doesn't support arbitrary objects, but here it is always assumed an object is passed in and should be serialized as JSON.

abernix · 2021-02-22T14:48:58Z

gateway-js/src/index.ts

@@ -863,4 +865,5 @@ export {
  Experimental_CompositionInfo,
 };

+export * from './QueryPlan';


In the past, this export * pattern has led to over-exporting and over-exposing internal details which often end up being used by consumers of this package. Apollo Server is a really strong example of how that can end up! Auto-complete makes it very easy for users to find these things and it tends to make the auto-complete more noisy rather than just showing the most user-applicable bits (i.e. when importing the package) which also makes it more likely that someone might import something that's an implementation detail inadvertently.

If the motivation for export * is entirely for testing, might we consider a pattern that avoids exporting all of QueryPlan, similar to what I did in apollo-cache-control in apollographql/apollo-server@68cbc93#diff-c209a58137b4a9da2beba783d377bff6c9c057208251f4dbea56763751680a80R442-R445 of apollographql/apollo-server#3997 where I used a __testing__ object to expose things that were specifically for testing? This groups all the testing stuff together in auto-complete, and also provides a more clear understanding to the user that this is not a public API.

If this is not for testing, but instead intended to surface public API, could we be explicit about what we're intending on exporting so that newly-exported properties of the ./QueryPlan package don't get inadvertently exported and polluting the root module?

abernix · 2021-02-22T14:50:10Z

package.json

@@ -47,7 +47,7 @@
    "bunyan": "1.8.14",
    "codecov": "3.7.2",
    "deep-freeze": "0.0.1",
-    "graphql": "14.7.0",
+    "graphql": "15.5.0",


Fwiw, I just updated Renovate to start actually bumping package dependencies on this repo. We should land this PR before this Saturday, otherwise there will be a number of merge conflicts. 😉

federation-js/src/__tests__/testSetup.ts

abernix · 2021-02-22T14:52:55Z

gateway-js/src/__tests__/gateway/buildService.test.ts

@@ -1,5 +1,5 @@
 import gql from 'graphql-tag';
-import { fetch } from '__mocks__/apollo-server-env';
+import { fetch } from '../../__mocks__/apollo-server-env';


I'm quite pleased to see the module mapping stuff go away, personally! ✨

This adds `@apollo/query-planner` as a front to `@apollo/query-planner-wasm`, re-exporting the functions in there with proper typings to avoid packages unnecessarily relying on `@apollo/gateway` just to be able to import `QueryPlan` and related types.

…y-planner`

Since `request.variables` has to be an object, this avoids stringifying in favor of setting an explicit different value. That seems to be in the spirit of this test.

…pendencies

We may want to look into adapting the types in `ts-jest` for this: kulshekhar/ts-jest/blob/master/src/utils/testing.ts

Updating `apollo-server-testing` without updating the `apollo-server-core` dependency meant `apollo-server-testing` depended on a newer version of `apollo-server-core,` and this lead to typing conflicts.

abernix

LGTM! I left a few final comments within.

abernix · 2021-03-02T12:54:23Z

gateway-js/src/make-fetch-happen.d.ts

@@ -52,7 +52,7 @@ declare module 'make-fetch-happen' {
  }

  let fetch: Fetcher & {
-    defaults(opts?: RequestInit & FetcherOptions): Fetcher;
+    defaults(opts?: RequestInit & FetcherOptions): typeof fetch;


Just a note: We should try switching gateway-js to use the now-published @types/make-fetch-happen package on DefinitelyTyped.

cc @trevor-scheer

abernix · 2021-03-02T12:55:52Z

package-lock.json

-    "@types/graphql-upload": {
-      "version": "8.0.4",
-      "resolved": "https://registry.npmjs.org/@types/graphql-upload/-/graphql-upload-8.0.4.tgz",
-      "integrity": "sha512-0TRyJD2o8vbkmJF8InppFcPVcXKk+Rvlg/xvpHBIndSJYpmDWfmtx/ZAtl4f3jR2vfarpTqYgj8MZuJssSoU7Q==",
-      "requires": {
-        "@types/express": "*",
-        "@types/fs-capacitor": "*",
-        "@types/koa": "*",
-        "graphql": "^15.3.0"
-      },
-      "dependencies": {
-        "graphql": {
-          "version": "15.4.0",
-          "resolved": "https://registry.npmjs.org/graphql/-/graphql-15.4.0.tgz",
-          "integrity": "sha512-EB3zgGchcabbsU9cFe1j+yxdzKQKAbGUWRb13DsrsMN1yyfmmIq+2+L5MqVWcDCE4V89R5AyUOi7sMOGxdsYtA=="
-        }
-      }
-    },


Hmm. I am skeptical that this removal is what we want. Maybe try unstaging this change and run npm run clean && npm i && npm i to see what happens (the extra install has certainly helped me in the past to solidify what happened in node_modules)?

AS 2.21 replaced both graphql-upload and @types/graphql-upload with a fork that has types built in, so that's probably what's causing this?

(I think the fork isn't being added in this PR because it was already in package-lock because there were some uses of AS 2.21 and some uses of older AS in package-lock already? Or just that npm hadn't quite deduped yet?)

abernix · 2021-03-02T12:57:51Z

package-lock.json

-    "graphql-upload": {
-      "version": "8.1.0",
-      "resolved": "https://registry.npmjs.org/graphql-upload/-/graphql-upload-8.1.0.tgz",
-      "integrity": "sha512-U2OiDI5VxYmzRKw0Z2dmfk0zkqMRaecH9Smh1U277gVgVe9Qn+18xqf4skwr4YJszGIh7iQDZ57+5ygOK9sM/Q==",
-      "requires": {
-        "busboy": "^0.3.1",
-        "fs-capacitor": "^2.0.4",
-        "http-errors": "^1.7.3",
-        "object-path": "^0.11.4"
-      }
-    },


Similarly uncertain about this removal.

abernix · 2021-03-02T13:01:04Z

query-planner-js/jest.config.js

+/** @typedef {import('ts-jest/dist/types')} */
+/** @type {import('@jest/types').Config.InitialOptions} */


This is really nice.

abernix · 2021-03-02T17:51:37Z

tsconfig.base.json

@@ -17,6 +17,7 @@
    "noUnusedLocals": true,
    "forceConsistentCasingInFileNames": true,
    "lib": ["es2019", "esnext.asynciterable"],
+    "skipLibCheck": true,


It's not clear what the motivation for this is or if this could be applied more granularly, e.g., at a specific package's own tsconfig.json which inherit from this base config.

I'm curious what the motivation is, but also just wanted to point out an often-unknown detail that tsconfig.json files are parsed with a jsonc parser which supports comments. Basically, I think a comment here noting the motivation for that would survive in this file would be really beneficial. (e.g., is it for performance, to allow the application to compile, etc?)

Because `apollo-link-http-common` expects to run in a web environment, it imports the `fetch` type from `WindowOrWorkerGlobalScope`. We should probably either fix that in `apollo-link-http-common` itself, or add `WindowOrWorkerGlobalScope` to `apollo-server-env` like we've done here.

abernix · 2021-03-04T10:31:02Z

Just to follow-up on the TODO in the PR text above, I've actioned the removal of the exception to Renovate in b6e3bad.

This is a backport from #453. It appears that when `@apollo/federation` is published without this and combined with `graphql@15` in a TypeScript setting, compilation fails; with this upgrade, the produced `.d.ts` files are compatible with both v14 and v15. See #537. Doing this as a cherry-pick on a release branch to avoid no-op publishes of `@apollo/gateway`.

Follows up on #453

@martijnwalraven

In #453 @martijnwalraven noted that `diagnostics` was still set to false (which allows `jest` to run tests even if typechecking fails) with errors logged. It seems like perhaps the errors in question have been fixed since then, as tests pass with `diagnostics: false` removed.

Something like this was intended to be part of #453. Because we have `diagnostics: false` in jest.config.base.js, typechecking failures in ts-jest don't actually stop the tests from running successfully; this change makes us typecheck the tests before we run them. (If you really want to run tests that don't typecheck you can run `npx jest` directly.) As part of this, fix a typechecking failure caused by the upgrade in PR #716 and run `npm dedup` to fix another one. We may want to consider finding a way to be comfortable removing `diagnostics: false` instead.

Something like this was intended to be part of #453. Because we have `diagnostics: false` in jest.config.base.js, typechecking failures in ts-jest don't actually stop the tests from running successfully; this change makes us typecheck the tests before we run them. (If you really want to run tests that don't typecheck you can run `npx jest` directly.) As part of this, fix a typechecking failure caused by the upgrade in PR #716 and run `npm dedup` to fix another typechecking failure. The `npm dedup` itself causes a snapshot to need to be updated (as the snapshot was created by some code that was still running a nested apollo-server 2.23). We may want to consider finding a way to be comfortable removing `diagnostics: false` instead as in #661, but we'll need to fix #662 before we can do that.

martijnwalraven added 20 commits February 4, 2021 12:06

Update Jest and related packages to latest versions

08e1b3a

Rename ts-jest tsConfig option to tsconfig

1f470ad

Update TypeScript to 4.1.3

806b959

TypeScript 4.0 introduced support for `noEmit` with composite projects, which is needed to make our test project configurations valid.

Add a TypeScript watch task to VS Code

cf7368e

Running this task watches both regular source files and test files, and incrementally compiles any changed files to show errors and warnings in the problem panel.

Rely on queryPlanSerializer instead of pretty printing the query pl…

22395b6

…an manually This seems to have been a contributor PR that slipped through, because we use `queryPlanSerializer` everywhere else.

Update ts-jest to latest version

6d338b9

Update graphql-js to v15

6304f18

Our peer dependencies still support v14, but this ensures we test against the latest version and also avoid a typing error because `specifiedByUrl` was only added in v15.

Remove unnecessary declaration of addSnapshotSerializer

894cfed

Fix typings for toHaveFetched matchers

12ad3bf

Clean up toMatchAST and add toMatchQueryPlan

d5c4e71

Simplify Jest configs and ensure testSetup.js is included in every …

912716d

…test `tsconfig.json`

Use tsconfig.test.json from ts-jest instead of root __tests__ one

5192e3f

Avoid path mapping for mocks and fix typing issues

9d75cb8

abernix marked this pull request as ready for review February 9, 2021 15:08

abernix requested review from trevor-scheer, glasser and queerviolet February 9, 2021 15:08

trevor-scheer approved these changes Feb 9, 2021

View reviewed changes

abernix assigned martijnwalraven Feb 10, 2021

abernix approved these changes Feb 22, 2021

View reviewed changes

martijnwalraven added 2 commits February 26, 2021 14:23

Add @apollo/query-planner package

2b3af8a

This adds `@apollo/query-planner` as a front to `@apollo/query-planner-wasm`, re-exporting the functions in there with proper typings to avoid packages unnecessarily relying on `@apollo/gateway` just to be able to import `QueryPlan` and related types.

Move prettyFormatQueryPlan and related serializers to `@apollo/quer…

d9bdbc6

…y-planner`

martijnwalraven added 11 commits March 2, 2021 10:48

Avoid stringifying variables in test because that breaks typing

2b774f9

Since `request.variables` has to be an object, this avoids stringifying in favor of setting an explicit different value. That seems to be in the spirit of this test.

Add resolver map type to test

c0cb248

Cast to any to allow arbitrary property mutation in test

b76f8c4

Fix make-fetch-happen types to allow defaults() chaining

0bd250f

Change unused parameter to _ to avoid type error

646230d

Set skipLibCheck to true in tsconfig to avoid type errors from de…

21dc682

…pendencies

Fix typing for Logger mock

9d21dd5

We may want to look into adapting the types in `ts-jest` for this: kulshekhar/ts-jest/blob/master/src/utils/testing.ts

Add non-null assertion for known schema types in test

1b34ce5

Merge branch 'main' into mw/testing-improvements

68b27f3

Add non-null assertion to fix assignment from closure

042edda

Update apollo-server-core dependency in @apollo/gateway

729faad

Updating `apollo-server-testing` without updating the `apollo-server-core` dependency meant `apollo-server-testing` depended on a newer version of `apollo-server-core,` and this lead to typing conflicts.

abernix requested review from abernix and trevor-scheer March 2, 2021 15:23

abernix approved these changes Mar 2, 2021

View reviewed changes

martijnwalraven merged commit 933b6a2 into main Mar 3, 2021

martijnwalraven deleted the mw/testing-improvements branch March 3, 2021 11:25

glasser mentioned this pull request Mar 5, 2021

[@apollo/federation 0.21.2] types.d.ts uses GraphQLObjectType with 3 instead 2 type parameters #537

Closed

abernix added a commit that referenced this pull request Mar 10, 2021

chore: Add query-planner-js to lerna.json and .prettierrc.

1810ad3

Follows up on #453

This was referenced Mar 11, 2021

chore(deps-dev): bump @apollo/federation from 0.21.0 to 0.22.0 Davide-Gheri/nestjs-mercurius#55

Closed

Bump @apollo/federation from 0.20.2 to 0.22.0 piersroberts/strapi#14

Closed

dependabot bot mentioned this pull request Mar 18, 2021

build(deps): bump @apollo/gateway from 0.20.0 to 0.24.4 rufman/graphql-mesh#55

Closed

glasser mentioned this pull request Apr 16, 2021

jest: remove diagnostics: false #661

Closed

glasser mentioned this pull request May 3, 2021

Typecheck tests before running them #727

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Testing infrastructure improvements #453

Testing infrastructure improvements #453

martijnwalraven commented Feb 4, 2021 •

edited by abernix

trevor-scheer left a comment

trevor-scheer Feb 9, 2021

abernix Feb 22, 2021

glasser commented Feb 9, 2021

abernix left a comment

abernix Feb 22, 2021

abernix Feb 22, 2021

martijnwalraven Mar 2, 2021

martijnwalraven Mar 2, 2021

abernix Feb 22, 2021

abernix Feb 22, 2021

abernix Feb 22, 2021

abernix left a comment

abernix Mar 2, 2021

abernix Mar 2, 2021

glasser Mar 2, 2021

glasser Mar 2, 2021

abernix Mar 2, 2021

glasser Mar 2, 2021

abernix Mar 2, 2021

abernix Mar 2, 2021

abernix commented Mar 4, 2021

		/** @typedef {import('ts-jest/dist/types')} */
		/** @type {import('@jest/types').Config.InitialOptions} */

Testing infrastructure improvements #453

Testing infrastructure improvements #453

Conversation

martijnwalraven commented Feb 4, 2021 • edited by abernix

TODO

Update Jest and related packages to latest versions, update TypeScript to latest version

Ensure are all our test files are type checked

Move snapshot serializers and matchers to federation-integration-testsuite

Fix Jest snapshot indentation for graphql-js AST

Remove use of moduleNameMapper for local imports

trevor-scheer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glasser commented Feb 9, 2021

abernix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abernix left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abernix commented Mar 4, 2021

martijnwalraven commented Feb 4, 2021 •

edited by abernix

Move snapshot serializers and matchers to `federation-integration-testsuite`

Fix Jest snapshot indentation for `graphql-js` AST

Remove use of `moduleNameMapper` for local imports