test(cli): rework snapshots to support parallel test execution

[bajtos]

Rework snapshot helper API from a global snapshot matcher cached via require.cache into per-test-file snapshot matchers. Implementation-wise, I am changing snapshots.js from exporting snapshot-related APIs to export a factory/initialization function instead. This is forcing all test files to explicitly initialize the matcher, which ensures that hooks are correctly installed even when running the tests in parallel.

-const {assertFilesToMatchSnapshot} = require('../../snapshots');
+const {assertFilesToMatchSnapshot} = require('../../snapshots')();

This PR is an alternate solution to 20da28e (see #5011).

• The first two commits will be removed from this pull request before landing, they are needed to enable parallel test execution to allow us to verify that my proposed changes work as intended.
• The last commit is best viewed with white-space changes ignored (direct link for your convenience).

Checklist

👉 Read and sign the CLA (Contributor License Agreement) 👈

• npm test passes on your machine
• New tests added or existing tests modified to cover all changes
• Code conforms with the style guide
• API Documentation in code was updated
• Documentation in /docs/site was updated
• Affected artifact templates in packages/cli were updated
• Affected example projects in examples/* were updated

👉 Check out how to submit a PR 👈

[raymondfeng]

I believe I tried this approach at the beginning and it didn't work as the beforeEach hooks created inside the factory function are ignored by mocha. Maybe the underlying issue was fixed.

I have some concerns here as https://github.com/strongloop/loopback-next/blob/59834fa6c1c6f181dc01f3c988da053f24fd4b1e/packages/cli/test/snapshot-matcher.js#L71 depends on when the file is required. There is a possibility that two test files are run in the same process by Mocha but both files are required in the first run. In that case, beforeEach is no longer invoked for the second run and currentTest won't be set correctly.

[bajtos]

I have some concerns here as

https://github.com/strongloop/loopback-next/blob/59834fa6c1c6f181dc01f3c988da053f24fd4b1e/packages/cli/test/snapshot-matcher.js#L71

depends on when the file is required. There is a possibility that two test files are run in the same process by Mocha but both files are required in the first run. In that case, beforeEach is no longer invoked for the second run and currentTest won't be set correctly.

I think this is misunderstanding. beforeEach hook is called from initializeSnapshots, not at the require time.

function initializeSnapshots(snapshotDir) {
  // ...
  beforeEach(setupSnapshots);
  // ...
}

The code in snapshot-matcher.ts has been always designed this way, I was originally thinking about extracting this file into a standalone npm package.

The problem was introduced when I added CLI-specific snapshots.js where I realized that I can call initializeSnapshots at require time and export snapshot matchers via module.exports. (And that's the part which does not work in parallel mocha.)

This pull request is fixing the design so that snapshots.js are exporting a factory function too.

You can run the tests with DEBUG=test* and see that initializeSnapshots is correctly called for every test file using snapshots. (That's why this PR is adding the error-stack-based debug log message!)

[raymondfeng]

I was referring to the usage such as const {expectFileToMatchSnapshot} = require('../../snapshots')();. Even though require('../../snapshots') returns a factory, it's immediately executed when the test file is loaded.

I'm adding b107660 to show the problem I have in mind. It might be an edge case. We can remove the commit afterward.

[bajtos]

I was referring to the usage such as const {expectFileToMatchSnapshot} = require('../../snapshots')();. Even though require('../../snapshots') returns a factory, it's immediately executed when the test file is loaded.

I'm adding b107660 to show the problem I have in mind. It might be an edge case. We can remove the commit afterward.

I see. Originally, I wanted to call the factory from within a describe block but then realized it's unnecessary work. Your counter argument shows why it would be a better option.

// packages/cli/test/integration/dummy/test1.integration.js
const setupSnapshots = require('../../snapshots');

describe('test1', () => {
  const {expectToMatchSnapshot} = setupSnapshots();

   it('matches snapshot 1', () => {
     expectToMatchSnapshot('test1 result');
   });
 });

Are you ok to continue with my proposal and move snapshot initialization into describe blocks? Or is this becoming too complex and we should look for other options (e.g. something based on your mocha hooks).

[bajtos]

Oh, so actually it's enough to move snapshot setup into commonTest function. Considering that writing a test suite factory like this is an advanced topic, I think it's ok to require authors to have a deeper knowledge of snapshotting infrastructure.

 exports.commonTest = function () {
   const {expectToMatchSnapshot} = require('../../snapshots')();

   describe('common', () => {
     it('matches snapshot 1', function () {
       expectToMatchSnapshot('common result');
     });
   });
 };

[bajtos]

Thank you for the example in b107660, it helped a lot.

I added bb72889 to fix your shared test suite and update the snapshots accordingly.

[raymondfeng]

@bajtos CI is failing.

[bajtos]

Your test case @raymondfeng is very good, it uncovers subtle problems in the assumptions made by the current snapshot design.

Here is what happens:

• Worker 1 runs test1.integration.js file, which runs tests from common.integration.js. The snapshot matcher for common tests is using the filename common.integration.snapshots.js.
• Worker 2 runs test2.integration.js file, which runs tests from common.integration.js. The snapshot matcher for common tests is using the filename common.integration.snapshots.js.

There are two problems:

• Two workers processes are touching the same file, possibly at the same time. The content may get corrupted.
• Each worker has only a subset of snapshot data (from test1 or test2), neither of the workers can produce the full list of "common" snapshots.

(@raymondfeng I am not sure if the solution you proposed in 20da28e is actually addressing those problems too.)

I think the solution is to change the snapshot matcher to compute the snapshot file name based on the test file loaded by Mocha (test1.integration.js), not on the file name that triggered snapshot initialization (common.integration.js).

I am going to start with refactoring your dummy tests into a permanent part of @loopback/cli test suite, to ensure we keep this use case supported regardless of what solution we pick for running tests in parallel.

[bajtos]

I think the solution is to change the snapshot matcher to compute the snapshot file name based on the test file loaded by Mocha (test1.integration.js), not on the file name that triggered snapshot initialization (common.integration.js).

Interestingly enough, this is already happening (at least when the shared test suite is called from inside a describe() block).

The problem with my solution proposed here is that it registers two places that try to write to the same snapshot file - one snapshot matcher instance is created from test1.integration.js, another from common.integration.js and they both write to the file test1.integration.snapshots.js.

[bajtos]

Calling commonTest outside of any describe block does not make sense to me. It creates duplicate entries in the list of test cases. In my experience with shared test suites, a shared test suite is typically called from inside a describe block, to scope each instance of the suite to the particular scenario being tested.

For example:

https://github.com/strongloop/loopback-next/blob/ae6427322451c914ae54f44dbb656981e7fbbb81/acceptance/repository-cloudant/src/__tests__/cloudant-default-repository.acceptance.ts#L13-L20

[bajtos]

Opened #5803 to add debug logs + tests for shared test suite.

[raymondfeng]

@bajtos CI is still failing. Do you need to make further changes here? I'm willing to approve it as soon as the CI is green so that we can enable parallel testing soon.

From the technical perspective, I still prefer to have a better way to receive this.currentTest from mocha.

[bajtos]

Do you need to make further changes here?

I think my approach is a dead-end. I'd like to land #5803 instead, and then take a look if 20da28e supports this edge case you identified. If yes, then I'd like to take another look at your implementation to see if it's good as-is or what improvements should be made.

so that we can enable parallel testing soon.

+1 to enable parallel tests soon.

From the technical perspective, I still prefer to have a better way to receive this.currentTest from mocha.

Yeah, that would be best.