[Bug] `nyc-test-coverage` is excessively difficult to use correctly

[mjameswh]

Describe the bug

At the moment, using the @temporalio/nyc-test-coverage package to collect coverage data for Workflow tests is quite difficult and generally fragile. Though it works with appropriate setup (eg. we have a sample demonstrating that it works), some users are struggling at replicating that "winning combination" in their own project, despite considerable efforts and willingness to accept applicable constraints (eg. use Mocha or AVA, ts-node rather than ts-mocha or similar, pre-transpile TS code to pure-JS, hardcode workflow name in their test files to avoid importing wokrlfow.ts from tests, etc).

What need to be done

This situation can be improved from two sides:

1. Improve the tool itself so that it is less fragile / more resilient to changes in its runtime environment
2. Provide clear instructions on how to use it

[zijianzz]

Here is our version of story about this issue

What are you really trying to do?

Test @temporalio/nyc-test-coverage package to collect coverage data by using either Jest or Mocha

Describe the bug

Workflow example:

Jest test code

import { TestWorkflowEnvironment } from '@temporalio/testing';
import { Worker } from '@temporalio/worker';
import { WorkflowCoverage } from '@temporalio/nyc-test-coverage';
import * as activities from '../../../activities';
import { exampleWorkflow } from '../test';

jest.mock('../../../activities');
const mockedActivties = jest.mocked(activities);
async function setupTestEnv(timeSkipping: boolean = true): Promise<TestWorkflowEnvironment> {
  return timeSkipping ? TestWorkflowEnvironment.createTimeSkipping() : TestWorkflowEnvironment.createLocal();
}

async function setupTestWorker(env: TestWorkflowEnvironment, workflowCoverage: WorkflowCoverage) {
  return Worker.create(
    workflowCoverage.augmentWorkerOptions({
      connection: env.nativeConnection,
      workflowsPath: require.resolve('../../test'),
      activities,
      taskQueue: 'test',
    }),
  );
}

describe('create restaurant', () => {
  let testEnv: TestWorkflowEnvironment;
  let worker: Worker;
  const workflowCoverage = new WorkflowCoverage();

  beforeAll(async () => {
    testEnv = await setupTestEnv();
  });

  afterAll(async () => {
    await testEnv?.teardown();
    jest.setTimeout(5000);
    jest.clearAllMocks();
    jest.resetAllMocks();
    workflowCoverage.mergeIntoGlobalCoverage();
  });

  beforeEach(async () => {
    worker = await setupTestWorker(testEnv, workflowCoverage);
  });

  describe('Successful case', () => {
    it('should return true', async () => {
      mockedActivties.testActivity.mockResolvedValueOnce(undefined);
      const result = await worker.runUntil(async () => {
        return testEnv.client.workflow.execute(exampleWorkflow, {
          workflowId: 'a-fake-workflow-id',
          taskQueue: 'test',
          args: [
            {
              ok: true,
            },
          ],
        });
      });

      expect(result).toStrictEqual(true);
    });
  });
});

Mocha test code

import { Worker } from '@temporalio/worker';
import { TestWorkflowEnvironment } from '@temporalio/testing';
import * as assert from 'assert';
import { v4 as uuid } from 'uuid';
import { after, before, describe, it } from 'mocha';
import { WorkflowCoverage } from '@temporalio/nyc-test-coverage';
import { ActivityFunction } from '@temporalio/common';
import * as activities from '../../../activities';
import { exampleWorkflow } from '../test';

const workflowCoverage = new WorkflowCoverage();
const mockedActivities: Record<string, ActivityFunction> = {};
const activitiesNames = Object.keys(activities);
for (const name of activitiesNames) {
  mockedActivities[name] = async () => undefined;
}

describe('createRestaurant workflow', async function () {
  let worker: Worker;
  let env: TestWorkflowEnvironment;

  this.slow(10_000);
  this.timeout(20_000);
  before(async function () {
    // Filter INFO log messages for clearer test output
    env = await TestWorkflowEnvironment.createTimeSkipping();
  });

  beforeEach(async () => {
    worker = await Worker.create(
      workflowCoverage.augmentWorkerOptions({
        connection: env.nativeConnection,
        taskQueue: 'test',
        workflowsPath: require.resolve('../../'),
        activities: {
          ...mockedActivities,
        },
      }),
    );
  });

  after(async () => {
    await env?.teardown();
  });

  after(() => {
    workflowCoverage.mergeIntoGlobalCoverage();
  });

  it('sucessfully return true if ok is true', async () => {
    const result = await worker.runUntil(async () => {
      return env.client.workflow.execute(exampleWorkflow, {
        taskQueue: 'test',
        workflowId: uuid(),
        args: [
          {
            ok: true,
          },
        ],
      });
    });
    assert.deepStrictEqual(result, true);
  });

  it('sucessfully return true if ok is false', async () => {
    const result = await worker.runUntil(async () => {
      return env.client.workflow.execute(exampleWorkflow, {
        taskQueue: 'test',
        workflowId: uuid(),
        args: [
          {
            ok: false,
          },
        ],
      });
    });
    assert.deepStrictEqual(result, true);
  });
});

By using jest with following jest config + @temporalio/nyc-test-coverage, sdk will send us runtime js error depending on version of sdk we used

module.exports = {
  testEnvironment: 'node',
  forceExit: true,
  collectCoverage: true,
  coverageDirectory: 'coverage/jest',
  coverageProvider: 'babel',
  roots: ['<rootDir>/src'],
  preset: 'ts-jest',
  moduleNameMapper: {
    '^@lafourchette/tfm-restaurant-composite-types/dist/(.*)$': '<rootDir>/packages/types/src/$1',
  },
  testMatch: ['**/__tests__/**/*.+(ts|tsx|js)', '**/?(*.)+(spec|test).+(ts|tsx|js)', '!**/__tests__/**/*.mocha.test.+(ts|tsx|js)'],
};

• sdk version 1.8.4 or 1.7.4 + jest => runtime js error

Error: Invalid file coverage object, missing keys, found:data
    at assertValidObject (/Users/zzuo/Git/tfm-restaurant-composite/node_modules/istanbul-lib-coverage/lib/file-coverage.js:38:15)
    at new FileCoverage (/Users/zzuo/Git/tfm-restaurant-composite/node_modules/istanbul-lib-coverage/lib/file-coverage.js:119:9)
    at maybeConstruct (/Users/zzuo/Git/tfm-restaurant-composite/node_modules/istanbul-lib-coverage/lib/coverage-map.js:15:12)
    at /Users/zzuo/Git/tfm-restaurant-composite/node_modules/istanbul-lib-coverage/lib/coverage-map.js:25:19
    at Array.forEach (<anonymous>)
    at loadMap (/Users/zzuo/Git/tfm-restaurant-composite/node_modules/istanbul-lib-coverage/lib/coverage-map.js:24:28)
    at new CoverageMap (/Users/zzuo/Git/tfm-restaurant-composite/node_modules/istanbul-lib-coverage/lib/coverage-map.js:42:25)
    at maybeConstruct (/Users/zzuo/Git/tfm-restaurant-composite/node_modules/istanbul-lib-coverage/lib/coverage-map.js:15:12)
    at CoverageMap.merge (/Users/zzuo/Git/tfm-restaurant-composite/node_modules/istanbul-lib-coverage/lib/coverage-map.js:53:23)
    at CoverageReporter.onTestResult (/Users/zzuo/Git/tfm-restaurant-composite/node_modules/@jest/reporters/build/CoverageReporter.js:167:25)
    at ReporterDispatcher.onTestFileResult (/Users/zzuo/Git/tfm-restaurant-composite/node_modules/@jest/core/build/ReporterDispatcher.js:32:24)
    at onResult (/Users/zzuo/Git/tfm-restaurant-composite/node_modules/@jest/core/build/TestScheduler.js:154:7)

By using mocha and launching following command + @temporalio/nyc-test-coverage, sdk will send us either inaccurate coverage data, timeout error depending on version of sdk we used

nyc --reporter=lcov --reporter=text ts-mocha src/**/*.mocha.test.ts

• 1.7.4 => timeout error

Environment/Versions

• OS and processor: [e.g. M1 Mac, x86 Windows, Linux]
• Temporal Version: [e.g. 1.14.0?] and/or SDK version
• Are you using Docker or Kubernetes or building Temporal from source?

Mac os 13.5 SDK 1.7.4 or 1.8.4

Additional context

[jamespsterling]

I had the same Error: Invalid file coverage object, missing keys, found:data error and fixed it by patching istanbul-lib-coverage.

diff --git a/node_modules/istanbul-lib-coverage/lib/file-coverage.js b/node_modules/istanbul-lib-coverage/lib/file-coverage.js
index 4ed4c09..08e272c 100644
--- a/node_modules/istanbul-lib-coverage/lib/file-coverage.js
+++ b/node_modules/istanbul-lib-coverage/lib/file-coverage.js
@@ -206,6 +206,8 @@ class FileCoverage {
             this.data = emptyCoverage(pathOrObj, reportLogic);
         } else if (pathOrObj instanceof FileCoverage) {
             this.data = pathOrObj.data;
+        } else if (typeof pathOrObj === 'object' && pathOrObj.data) {
+            this.data = pathOrObj.data;
         } else if (typeof pathOrObj === 'object') {
             this.data = pathOrObj;
         } else {

The underlying issue is that FileCoverage is receiving an instance of CoverageMap when it's expecting instance of FileCoverage in the constructor branching assignment to this.data.

https://github.com/istanbuljs/istanbuljs/blob/5584b50305a6a17d3573aea25c84e254d4a08b65/packages/istanbul-lib-coverage/lib/file-coverage.js#L205-L213

[mjameswh]

@jamespsterling Thanks for sharing your findings. Yes, I believe there's been a recent regression (possibly on our side) that made our nyc-test-coverage plugin fails in cases where it was previously working. What you are pointing at should help figure out where that regression lies.

[ilijaNL]

Any update on this?

[mjameswh]

Any update on this?

Not yet, sorry.

[ilijaNL]

This area needs some improvement. We use a monorepo setup with jest (ts-jest as transformer). We have high code coverage requirement thus we using collectCoverageFrom. Somehow when we use the nyc-test-coverage package, the coverage is not merged correctly and we get really weird code coverage report. I assume this is due that the final sourcemaps are different between what ts-jest transforms and webpack (used by temporal sdk). This is kind of deal breaker atm.

[surjyams]

Is there any update on this? We are running into the same issue with @temporalio/[email protected]

[gaurav1999]

@mjameswh Facing same issue

● Test suite failed to run

    Invalid file coverage object, missing keys, found:data

      at assertValidObject (../node_modules/istanbul-lib-coverage/lib/file-coverage.js:38:15)
      at new FileCoverage (../node_modules/istanbul-lib-coverage/lib/file-coverage.js:214:9)
      at maybeConstruct (../node_modules/istanbul-lib-coverage/lib/coverage-map.js:15:12)
      at ../node_modules/istanbul-lib-coverage/lib/coverage-map.js:25:19
          at Array.forEach (<anonymous>)
      at loadMap (../node_modules/istanbul-lib-coverage/lib/coverage-map.js:24:28)
      at new CoverageMap (../node_modules/istanbul-lib-coverage/lib/coverage-map.js:42:25)
      at maybeConstruct (../node_modules/istanbul-lib-coverage/lib/coverage-map.js:15:12)

 const worker = await Worker.create(workflowCoverage.augmentWorkerOptions({
      connection: nativeConnection,
      taskQueue,
      workflowsPath: require.resolve('./runops-success.test.ts'),
      activities
    }));

[0xBigBoss]

I had the same Error: Invalid file coverage object, missing keys, found:data error and fixed it by patching istanbul-lib-coverage.

diff --git a/node_modules/istanbul-lib-coverage/lib/file-coverage.js b/node_modules/istanbul-lib-coverage/lib/file-coverage.js
index 4ed4c09..08e272c 100644
--- a/node_modules/istanbul-lib-coverage/lib/file-coverage.js
+++ b/node_modules/istanbul-lib-coverage/lib/file-coverage.js
@@ -206,6 +206,8 @@ class FileCoverage {
             this.data = emptyCoverage(pathOrObj, reportLogic);
         } else if (pathOrObj instanceof FileCoverage) {
             this.data = pathOrObj.data;
+        } else if (typeof pathOrObj === 'object' && pathOrObj.data) {
+            this.data = pathOrObj.data;
         } else if (typeof pathOrObj === 'object') {
             this.data = pathOrObj;
         } else {

The underlying issue is that FileCoverage is receiving an instance of CoverageMap when it's expecting instance of FileCoverage in the constructor branching assignment to this.data.

https://github.com/istanbuljs/istanbuljs/blob/5584b50305a6a17d3573aea25c84e254d4a08b65/packages/istanbul-lib-coverage/lib/file-coverage.js#L205-L213

This patch is working for me.

Side note, this seems it's also being tracked in the nyc github. https://github.com/istanbuljs/nyc/issues/1226

[jgnieuwhof]

@ilijaNL

... Somehow when we use the nyc-test-coverage package, the coverage is not merged correctly and we get really weird code coverage report. I assume this is due that the final sourcemaps are different between what ts-jest transforms and webpack (used by temporal sdk). This is kind of deal breaker atm.

I found the same thing - I was getting coverage but the lines didn't make sense, and were actually inconsistent depending on which test files I included in the test run. The issue does appear to be with how the mergeIntoGlobalCoverage function performs its merge, as coverage is correct when bypassing the merge and just writing the coverage data to .nyc_output.

Here's the workaround that solved the issue for me:

// instead of this
afterAll(async () => {
  workflowCoverage.mergeIntoGlobalCoverage();
});

// do this
import { createCoverageMap } from "istanbul-lib-coverage";

afterAll(async () => {
  const coverageMap = createCoverageMap();
  for (const data of workflowCoverage.coverageMapsData) {
    coverageMap.merge(data);
  }
  await fs.writeFile(
    path.join(".nyc_output", `${uuid4()}.json`),
    JSON.stringify(coverageMap)
  );
});

[ilijaNL]

@ilijaNL

... Somehow when we use the nyc-test-coverage package, the coverage is not merged correctly and we get really weird code coverage report. I assume this is due that the final sourcemaps are different between what ts-jest transforms and webpack (used by temporal sdk). This is kind of deal breaker atm.

I found the same thing - I was getting coverage but the lines didn't make sense, and were actually inconsistent depending on which test files I included in the test run. The issue does appear to be with how the mergeIntoGlobalCoverage function performs its merge, as coverage is correct when bypassing the merge and just writing the coverage data to .nyc_output.

Here's the workaround that solved the issue for me:

// instead of this
afterAll(async () => {
  workflowCoverage.mergeIntoGlobalCoverage();
});

// do this
import { createCoverageMap } from "istanbul-lib-coverage";

afterAll(async () => {
  const coverageMap = createCoverageMap();
  for (const data of workflowCoverage.coverageMapsData) {
    coverageMap.merge(data);
  }
  await fs.writeFile(
    path.join(".nyc_output", `${uuid4()}.json`),
    JSON.stringify(coverageMap)
  );
});

This is okaay if your whole describe block only include workflow tests, if it includes some non workflow test, jest (global.coverage) will not include those in the coverage.