CI: new job which fails if a file contains FIXME (#5121)

When working on a big PR, sometimes you have something you know you need to fix
before merging. With this CI job, just add a FIXME comment and you'll be forced
to address it before all CI can pass.

We intentionally allow CI to make tarballs for a commit and publish npm packages
even with FIXME, because you may want to test out a build of an incomplete PR.

Change existing TODO comments to FIXME. Document this requirement in
CONTRIBUTING.md and make the text about PRs with failing tests a bit gentler.

Author: glasser

.circleci/config.yml

@@ -72,6 +72,24 @@ jobs:
     steps:
       - common_test_steps
 
+  # We make CI fail if any file contains FIX and ME right next to each other.
+  # This means that when working on a big PR, you can throw in notes that you
+  # are forced to come back to before merging the PR.  Note that we don't block
+  # the tarball or publish steps on this, since you may want to try out builds
+  # on branches that still contain unresolved problems. In order for this file
+  # to not cause this job to fail, we obfuscate the target string by encoding
+  # the last letter in in hex (\x45 = E).
+  "Check for FIXM\x45":
+    docker:
+    - image: cimg/base:stable
+    steps:
+    - add_ssh_keys
+    - checkout
+    - run:
+        name: "Check for FIXM\x45"
+        # ! means this fails if git grep succeeds, ie if there are any matches
+        command: "! git grep FIXM\x45"
+
 # XXX We used to use this filter to only run a "Docs" job on docs branches.
 #     Now we use it to disable all jobs. It's unclear if there's a simpler way
 #     to do this!
@@ -98,8 +116,6 @@ workflows:
   version: 2
   Build:
     jobs:
-      # - NodeJS 6:
-      #     <<: *common_non_publish_filters
       - NodeJS 8:
           <<: *common_non_publish_filters
       - NodeJS 10:
@@ -108,6 +124,7 @@ workflows:
           <<: *common_non_publish_filters
       - NodeJS 14:
           <<: *common_non_publish_filters
+      - "Check for FIXM\x45"
       - oss/lerna_tarballs:
           name: Package tarballs
           <<: *common_non_publish_filters

CONTRIBUTING.md

@@ -74,7 +74,7 @@ For significant changes to a repository, it’s important to settle on a design
 
 It’s important that every piece of code in Apollo packages is reviewed by at least one core contributor familiar with that codebase. Here are some things we look for:
 
-1.  **Required CI checks pass.** This is a prerequisite for the review, and it is the PR author's responsibility. As long as the tests don’t pass, the PR won't get reviewed.
+1.  **Required CI checks pass.** This is a prerequisite for the review, and it is the PR author's responsibility. As long as the tests don’t pass, the PR won't get merged. If you're having a challenge getting tests to pass, feel free to ask for help. In addition to running tests, CI will fail if you add the words FIX and ME without a space within them in any file; this lets you add comments during the development process that you can't forget to address before merging.
 2.  **Simplicity.** Is this the simplest way to achieve the intended goal? If there are too many files, redundant functions, or complex lines of code, suggest a simpler way to do the same thing. In particular, avoid implementing an overly general solution when a simple, small, and pragmatic fix will do.
 3.  **Testing.** Do the tests ensure this code won’t break when other stuff changes around it? When it does break, will the tests added help us identify which part of the library has the problem? Did we cover an appropriate set of edge cases? Look at the test coverage report if there is one. Are all significant code paths in the new code exercised at least once?
 4.  **No unnecessary or unrelated changes.** PRs shouldn’t come with random formatting changes, especially in unrelated parts of the code. If there is some refactoring that needs to be done, it should be in a separate PR from a bug fix or feature, if possible.

packages/apollo-cache-control/src/index.ts

@@ -25,7 +25,7 @@ export enum CacheScope {
 
 export interface CacheControlExtensionOptions {
   defaultMaxAge?: number;
-  // FIXME: We should replace these with
+  // TODO: We should replace these with
   // more appropriately named options.
   calculateHttpHeaders?: boolean;
   stripFormattedExtensions?: boolean;

packages/apollo-datasource-rest/src/__tests__/RESTDataSource.test.ts

@@ -213,7 +213,7 @@ describe('RESTDataSource', () => {
       await dataSource.getFoo();
 
       expect(fetch).toBeCalledTimes(1);
-      // FIXME: request.credentials is not supported by node-fetch
+      // TODO: request.credentials is not supported by node-fetch
       // expect(fetch.mock.calls[0][0].credentials).toEqual('include');
     });
 

packages/apollo-server-cache-memcached/src/index.ts

@@ -6,7 +6,7 @@ import Memcached from 'memcached';
 import { promisify } from 'util';
 
 export class MemcachedCache implements TestableKeyValueCache {
-  // FIXME: Replace any with proper promisified type
+  // TODO: Replace any with proper promisified type
   readonly client: any;
   readonly defaultSetOptions: KeyValueCacheSetOptions = {
     ttl: 300,

packages/apollo-server-caching/src/InMemoryLRUCache.ts

@@ -14,7 +14,7 @@ function defaultLengthCalculation(item: any) {
 export class InMemoryLRUCache<V = string> implements TestableKeyValueCache<V> {
   private store: LRUCache<string, V>;
 
-  // FIXME: Define reasonable default max size of the cache
+  // TODO: Define reasonable default max size of the cache
   constructor({
     maxSize = Infinity,
     sizeCalculator = defaultLengthCalculation,

packages/apollo-server-core/src/ApolloServer.ts

@@ -1279,7 +1279,7 @@ export class ApolloServerBase {
     if (typeof options.context === 'function') {
       options.context = (options.context as () => never)();
     } else if (typeof options.context === 'object') {
-      // FIXME: We currently shallow clone the context for every request,
+      // TODO: We currently shallow clone the context for every request,
       // but that's unlikely to be what people want.
       // We allow passing in a function for `context` to ApolloServer,
       // but this only runs once for a batched request (because this is resolved

packages/apollo-server-core/src/requestPipeline.ts

@@ -222,7 +222,7 @@ export async function processGraphQLRequest<TContext>(
       metrics.persistedQueryRegister = true;
     }
   } else if (query) {
-    // FIXME: We'll compute the APQ query hash to use as our cache key for
+    // TODO: We'll compute the APQ query hash to use as our cache key for
     // now, but this should be replaced with the new operation ID algorithm.
     queryHash = computeQueryHash(query);
   } else {
@@ -332,7 +332,7 @@ export async function processGraphQLRequest<TContext>(
       }
     }
 
-    // FIXME: If we want to guarantee an operation has been set when invoking
+    // TODO: If we want to guarantee an operation has been set when invoking
     // `willExecuteOperation` and executionDidStart`, we need to throw an
     // error here and not leave this to `buildExecutionContext` in
     // `graphql-js`.

packages/apollo-server-core/src/runHttpQuery.ts

@@ -42,7 +42,7 @@ export interface ApolloServerHttpResponse {
 }
 
 export interface HttpQueryResponse {
-  // FIXME: This isn't actually an individual GraphQL response, but the body
+  // TODO: This isn't actually an individual GraphQL response, but the body
   // of the HTTP response, which could contain multiple GraphQL responses
   // when using batching.
   graphqlResponse: string;
@@ -132,7 +132,7 @@ export async function runHttpQuery(
     options.debug = debugDefault;
   }
 
-  // FIXME: Errors thrown while resolving the context in
+  // TODO: Errors thrown while resolving the context in
   // ApolloServer#graphQLServerOptions are currently converted to
   // a throwing function, which we invoke here to rethrow an HTTP error.
   // When we refactor the integration between ApolloServer, the middleware and
@@ -168,7 +168,7 @@ export async function runHttpQuery(
     executor: options.executor,
     fieldResolver: options.fieldResolver,
 
-    // FIXME: Use proper option types to ensure this
+    // TODO: Use proper option types to ensure this
     // The cache is guaranteed to be initialized in ApolloServer, and
     // cacheControl defaults will also have been set if a boolean argument is
     // passed in.
@@ -239,7 +239,7 @@ export async function processHTTPRequest<TContext>(
   function buildRequestContext(
     request: GraphQLRequest,
   ): GraphQLRequestContext<TContext> {
-    // FIXME: We currently shallow clone the context for every request,
+    // TODO: We currently shallow clone the context for every request,
     // but that's unlikely to be what people want.
     // We allow passing in a function for `context` to ApolloServer,
     // but this only runs once for a batched request (because this is resolved

packages/apollo-server-env/src/index.browser.js

@@ -40,6 +40,6 @@ if (!global.process.hrtime) {
 }
 
 if (!global.os) {
-  // FIXME: Add some sensible values
+  // TODO: Add some sensible values
   global.os = {};
 }