search

temporalio / sdk-typescript

Temporal TypeScript SDK
Other
513 stars 100 forks source link

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

Open mjameswh opened 1 year ago

mjameswh commented 1 year ago

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 commented 1 year ago

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:

image

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)'],
};

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

Environment/Versions

Mac os 13.5 SDK 1.7.4 or 1.8.4

Additional context

jamespsterling commented 9 months ago

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 commented 9 months ago

@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 commented 4 months ago

Any update on this?

mjameswh commented 4 months ago

Any update on this?

Not yet, sorry.

ilijaNL commented 4 months ago

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 commented 3 months ago

Is there any update on this? We are running into the same issue with @temporalio/nyc-test-coverage@1.10.1

gaurav1999 commented 2 months ago

@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 commented 2 months ago

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 commented 1 month ago

@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 commented 1 month ago

@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.