Merge pull request #16027 from backstage/rugvip/workers

cli: additional fix for worker memory limits to make sure it's always applied by default

.changeset/chatty-owls-care.md

@@ -2,4 +2,4 @@
 '@backstage/cli': patch
 ---
 
-The `backstage-cli repo test` command now sets a default Jest `--workerIdleMemoryLimit` of 1GB. This is the recommended workaround for dealing with Jest workers leaking memory and eventually hitting the heap limit.
+The `backstage-cli repo test` command now sets a default Jest `--workerIdleMemoryLimit` of 1GB. If needed to ensure that tests are not run in band, `--maxWorkers=2` is set as well. This is the recommended workaround for dealing with Jest workers leaking memory and eventually hitting the heap limit.

packages/cli/src/commands/repo/test.test.ts

@@ -0,0 +1,47 @@
+/*
+ * Copyright 2020 The Backstage Authors
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import { createFlagFinder } from './test';
+
+describe('createFlagFinder', () => {
+  it('finds flags', () => {
+    const find = createFlagFinder([
+      '--foo',
+      '--no-bar',
+      '-b',
+      '-c',
+      '--baz=1',
+      '--qux',
+      '2',
+      '-de',
+    ]);
+
+    expect(
+      find('--foo', '--bar', '-b', '-c', '--baz', '--qux', '-d', '-e'),
+    ).toBe(true);
+    expect(find('--foo')).toBe(true);
+    expect(find('--bar')).toBe(true);
+    expect(find('--no-bar')).toBe(false);
+    expect(find('-a')).toBe(false);
+    expect(find('-b')).toBe(true);
+    expect(find('-c')).toBe(true);
+    expect(find('-d')).toBe(true);
+    expect(find('-e')).toBe(true);
+    expect(find('--baz')).toBe(true);
+    expect(find('--qux')).toBe(true);
+    expect(find('--qux')).toBe(true);
+  });
+});

packages/cli/src/commands/repo/test.ts

@@ -14,18 +14,36 @@
  * limitations under the License.
  */
 
+import os from 'os';
 import { Command, OptionValues } from 'commander';
 import { PackageGraph } from '../../lib/monorepo';
 import { paths } from '../../lib/paths';
 import { runCheck } from '../../lib/run';
 
-function includesAnyOf(hayStack: string[], ...needles: string[]) {
-  for (const needle of needles) {
-    if (hayStack.includes(needle)) {
-      return true;
+export function createFlagFinder(args: string[]) {
+  const flags = new Set<string>();
+
+  for (const arg of args) {
+    if (arg.startsWith('--no-')) {
+      flags.add(`--${arg.slice('--no-'.length)}`);
+    } else if (arg.startsWith('--')) {
+      flags.add(arg.split('=')[0]);
+    } else if (arg.startsWith('-')) {
+      const shortFlags = arg.slice(1).split('');
+      for (const shortFlag of shortFlags) {
+        flags.add(`-${shortFlag}`);
+      }
     }
   }
-  return false;
+
+  return (...findFlags: string[]) => {
+    for (const flag of findFlags) {
+      if (flags.has(flag)) {
+        return true;
+      }
+    }
+    return false;
+  };
 }
 
 function removeOptionArg(args: string[], option: string) {
@@ -55,24 +73,19 @@ export async function command(opts: OptionValues, cmd: Command): Promise<void> {
   const allArgs = parent.args as string[];
   const args = allArgs.slice(allArgs.indexOf('test') + 1);
 
+  const hasFlags = createFlagFinder(args);
+
   // Only include our config if caller isn't passing their own config
-  if (!includesAnyOf(args, '-c', '--config')) {
+  if (!hasFlags('-c', '--config')) {
     args.push('--config', paths.resolveOwn('config/jest.js'));
   }
 
-  if (!includesAnyOf(args, '--no-passWithNoTests', '--passWithNoTests=false')) {
+  if (!hasFlags('--passWithNoTests')) {
     args.push('--passWithNoTests');
   }
 
   // Run in watch mode unless in CI, coverage mode, or running all tests
-  if (
-    !process.env.CI &&
-    !args.includes('--coverage') &&
-    // explicitly no watching
-    !includesAnyOf(args, '--no-watch', '--watch=false', '--watchAll=false') &&
-    // already watching
-    !includesAnyOf(args, '--watch', '--watchAll')
-  ) {
+  if (!process.env.CI && !hasFlags('--coverage', '--watch', '--watchAll')) {
     const isGitRepo = () =>
       runCheck('git', 'rev-parse', '--is-inside-work-tree');
     const isMercurialRepo = () => runCheck('hg', '--cwd', '.', 'root');
@@ -87,15 +100,27 @@ export async function command(opts: OptionValues, cmd: Command): Promise<void> {
   // When running tests from the repo root in large repos you can easily hit the heap limit.
   // This is because Jest workers leak a lot of memory, and the workaround is to limit worker memory.
   // We set a default memory limit, but if an explicit one is supplied it will be used instead
-  if (!args.some(arg => arg.match(/^--workerIdleMemoryLimit/))) {
+  if (!hasFlags('--workerIdleMemoryLimit')) {
     args.push('--workerIdleMemoryLimit=1000M');
   }
 
+  // In order for the above worker memory limit to work we need to make sure the worker
+  // count is set to at least 2, as the tests will otherwise run in-band.
+  // Depending on the mode tests are run with the default count is either cpus-1, or cpus/2.
+  // This means that if we've got at 4 or more cores we'll always get at least 2 workers, but
+  // otherwise we need to set the worker count explicitly unless already done.
+  if (
+    os.cpus().length <= 3 &&
+    !hasFlags('-i', '--runInBand', '-w', '--maxWorkers')
+  ) {
+    args.push('--maxWorkers=2');
+  }
+
   if (opts.since) {
     removeOptionArg(args, '--since');
   }
 
-  if (opts.since && !args.some(arg => arg.startsWith('--selectProjects'))) {
+  if (opts.since && !hasFlags('--selectProjects')) {
     const packages = await PackageGraph.listTargetPackages();
     const graph = PackageGraph.fromPackages(packages);
     const changedPackages = await graph.listChangedPackages({