salesforce / sfdx-lwc-jest

Run Jest against LWC components in SFDX workspace environment
MIT License
165 stars 82 forks source link

Error when testing parent and child components where child invokes an apex method #232

Closed filiprafalowicz closed 3 years ago

filiprafalowicz commented 3 years ago

Description

With auto-mock of Apex methods that was introduced and possibly module caching on jest side (https://github.com/facebook/jest/blob/master/packages/jest-resolve/src/resolver.ts#L158) if there is a parent and child component where child calls Apex imperatively mock from parent component will win and child component tests may fail. This occurs only if you run parent component test first and then child component test. If you run them in reverse order, all works fine as child component can mock the module properly.

Also, this caching also causes issues when the same module is imported in multiple unrelated components or when you use service components that only make Apex calls. If two components use the same service and tests are mocking its Apex call, there is a chance that mockResolvedValue or mockRejectedValue will work cross-tests meaning value we want to return in test for cmp1 will be returned in test for cmp2. This one is very hard to reproduce and happens when two test files are processed at the same time, but it is also related to the caching issue as they both share one instance of the mock.

Steps to Reproduce

cmpB (child component)

<template></template>
import { LightningElement, api } from 'lwc';
import myApexAction from '@salesforce/apex/MyApexCtrl.myApexAction';

export default class CmpB extends LightningElement {
    @api
    callApex() {
        myApexAction();
    }
}
// cmpB.test.js
import { createElement } from 'lwc';
import myApexAction from '@salesforce/apex/MyApexCtrl.myApexAction';
import CmpB from 'c/cmpB';

jest.mock(
    '@salesforce/apex/MyApexCtrl.myApexAction',
    () => {
        return {
            default: jest.fn()
        };
    },
    { virtual: true }
);

describe('c-cmp-b', () => {
    afterEach(() => {
        while (document.body.firstChild) {
            document.body.removeChild(document.body.firstChild);
        }
        jest.clearAllMocks();
    });

    it('should call myApexAction when callApex called', () => {
        myApexAction.mockResolvedValue();

        const element = createElement('c-cmp-b', {
            is: CmpB
        });

        document.body.appendChild(element);

        element.callApex();

        // Assertion below fails when cmpA tests run before cmpB tests
        expect(myApexAction).toHaveBeenCalledTimes(1);
    });
});

cmpA (parent component)

<template>
    <c-cmp-b></c-cmp-b>
</template>
import { LightningElement } from 'lwc';

export default class CmpA extends LightningElement {}
// cmpA.test.js
import { createElement } from 'lwc';
import CmpA from 'c/cmpA';

describe('c-cmp-a', () => {
    afterEach(() => {
        while (document.body.firstChild) {
            document.body.removeChild(document.body.firstChild);
        }
        jest.clearAllMocks();
    });

    it('should render cmpA', () => {
        const element = createElement('c-cmp-a', {
            is: CmpA
        });

        document.body.appendChild(element);
    });
});

Command to reproduce the issue

# Running tests in serial mode to easily reproduce the issue
sfdx-lwc-jest -- --runInBand --runTestsByPath ./force-app/main/default/lwc/cmpA/__tests__/cmpA.test.js ./force-app/main/default/lwc/cmpB/__tests__/cmpB.test.js

Note: issue happens only if cmpA test runs before cmpB test. I used this sample test sequencer to make sure that's the order:

const Sequencer = require('@jest/test-sequencer').default;

class CustomSequencer extends Sequencer {
    sort(tests) {
        const copyTests = Array.from(tests);
        return copyTests.sort((testA, testB) => (testA.path > testB.path ? 1 : -1));
    }
}

module.exports = CustomSequencer;

And then, added this to jest.config.js:

{
    "testSequencer": "<rootDir>/testSequencer.js"
}

Expected Results

Both tests are passing no matter what's the order.

Actual Results

If you run cmpB test first and then cmpA test, they both pass. If you change the order, only cmpA test passes.

Version

Additional context/Screenshots

I tried to use fix that is included in PR: https://github.com/salesforce/sfdx-lwc-jest/pull/231 to see if it fixes the issue as I have also encountered the issue described there, but those two are unrelated. This one seems to be strictly related to the mocks caching and sharing instance of mock between all of the tests.

jodarove commented 3 years ago

This is an interesting issue! Thanks for sharing your findings and the great repro steps @filiprafalowicz; they helped a lot.

As you mention, with v0.12.3, we started "auto-mocking" apex methods (at the resolver level), but before, they also were auto-mocked at the transform level (to function () { return Promise.resolve() }). The difference is that with 0.12.3, the @salesforce/apex/* modules stopped being virtual. The mock itself is a jest.fn() (which is easy to manipulate by test authors) and also a valid apex wire adapter (easier to test when used with @wire, check wire-jest-test-util).

In the repro, cmpB.test.js have (I assume it does because, before 0.12.3, there was no way to mock the return value of calling the method):

jest.mock(
    '@salesforce/apex/MyApexCtrl.myApexAction',
    () => {
        return {
            default: jest.fn()
        };
    },
    { virtual: true }
);

If { virtual: true } is removed (in 0.12.3 is not virtual), the tests should pass regarding the order. More than this, there is no need to mock the module anymore because (unless overwritten) by default, the method from @salesforce/apex/MyApexCtrl.myApexAction is a jest.fn(), so you can mock the resolved value, etc.

Looping @trevor-bliss and @pmdartus (the experts in jest) in case I missed something in my analysis.

filiprafalowicz commented 3 years ago

Hey @jodarove - thanks for your detailed response. When I removed my mock for the module both tests are passing now, which is great. One thing that comes to my mind regarding this is to include that in migration doc. I haven't seen it mentioned anywhere that this mocking is no longer needed.

So all would be good and resolved, but... I think something is still wrong with those auto-mocks. If you change cmpB to make two Apex calls instead of just one, then those auto-mocks are messed up and results are not correct. It is no longer related to the parent - child component issue, but if you run tests for cmpB only, you will see the errors. Here are new snippets for cmpB:

import { LightningElement, api } from 'lwc';
import myApexAction from '@salesforce/apex/MyApexCtrl.myApexAction';
import myAnotherApexAction from '@salesforce/apex/MyApexCtrl.myAnotherApexAction';

export default class CmpB extends LightningElement {
    @api
    async callApex() {
        const myApexActionResult = await myApexAction();
        const myAnotherApexActionResult = await myAnotherApexAction();

        return {
            myApexActionResult,
            myAnotherApexActionResult,
        };
    }
}
// cmpB.test.js
import { createElement } from 'lwc';
import myApexAction from '@salesforce/apex/MyApexCtrl.myApexAction';
import myAnotherApexAction from '@salesforce/apex/MyApexCtrl.myAnotherApexAction';
import CmpB from 'c/cmpB';

describe('c-cmp-b', () => {
    afterEach(() => {
        while (document.body.firstChild) {
            document.body.removeChild(document.body.firstChild);
        }
        jest.clearAllMocks();
    });

    it('should call both myApexAction and myAnotherApexAction when callApex called', async () => {
        myApexAction.mockResolvedValue();
        myAnotherApexAction.mockResolvedValue();

        const element = createElement('c-cmp-b', {
            is: CmpB
        });

        document.body.appendChild(element);

        await element.callApex();

        // First assertion fails with 2 calls received
        expect(myApexAction).toHaveBeenCalledTimes(1);
        expect(myAnotherApexAction).toHaveBeenCalledTimes(1);
    });

    it('should return correct result when callApex called', async () => {
        myApexAction.mockResolvedValue('myApexActionValue');
        myAnotherApexAction.mockResolvedValue('myAnotherApexActionValue');

        const element = createElement('c-cmp-b', {
            is: CmpB
        });

        document.body.appendChild(element);

        const result = await element.callApex();

        // Received object is: { myApexActionResult: "myAnotherApexActionValue", myAnotherApexActionResult: "myAnotherApexActionValue" }
        expect(result).toEqual({
            myApexActionResult: 'myApexActionValue',
            myAnotherApexActionResult: 'myAnotherApexActionValue',
        });
    });
});

So in this case, it seems that only one Apex mock is working and it is sending the same value for both Apex calls.

I tested it with the fix from https://github.com/salesforce/sfdx-lwc-jest/pull/231, but it didn't change anything - result is still wrong. On the other hand, if I keep my own, virtual mocks, all of the tests are passing, but if I keep it, I will face the issue I reported originally.

jodarove commented 3 years ago

mmm, great point. This use case wasn't analyzed when doing the auto mocking at the resolver level; I remember that @pmdartus had some pushbacks, but (IMHO) this is a show stopper unless we figure out a way to make expect(myApexAction).not.toBe(myAnotherApexAction); truthy.

@filiprafalowicz, let me discuss with the team, ill get back to you soon.

BatemanVO commented 3 years ago

Just want to add another voice here confirming the race condition failures @filiprafalowicz has reproduced so well. Our Org has about ~100 test files and, after updating from version 0.10 to 0.12 and modifying test files to remove all the registerTestWireAdapters, we're getting inconsistent results when running all tests.

Sometimes all tests pass with 100% coverage, sometimes we get a single failure (an uncaught Promise of cannot read property .then of undefined when referencing Apex class calls) in one test that will happen again on another run, but in another test, and sometimes we'll get multiple failures with less than 100% coverage. However, running tests one at a time, everything passes and has full coverage.

I also created a CustomSequencer class that extends TestSequencer and added the --runInBand flag to confirm that tests would run in the same order every time and can reproduce failures in the same manner as listed in previous comments. I noted that, for some tests, if the first time an Apex controller was mocked, I left { virtual: true }, but then removed that option from all subsequent tests that referenced the same Apex controller, they would sometimes work, but as also mentioned, it appears to mock the same value for all Apex returns, causing other tests to fail (notably when we're testing a mockResolvedValue and mockRejectedValue in the same test).

Without a workaround, our team is currently blocked on updating our package to the latest version.

yippie commented 3 years ago

Yea, after upgrading my sfdx project to v52 half of my tests suddenly broke and I am seeing data passed into the wrong wire method all over.

jodarove commented 3 years ago

This issue is resolved in v0.12.6 (summer21) and v0.14.0 (next). Please note that apex and apexContinuation methods are no longer auto-mocked, and test authors will need to keep implementing their own mocks.

FYI @filiprafalowicz, @BatemanVO, @yippie .