mantinedev / mantine

A fully featured React components library
https://mantine.dev
MIT License
26.12k stars 1.85k forks source link

@mantine/core/Combobox issue in Jest tests #6127

Open webdistortion opened 4 months ago

webdistortion commented 4 months ago

Dependencies check up

What version of @mantine/* packages do you have in package.json?

7.8.1

What package has an issue?

@mantine/core/Combobox

What framework do you use?

just plain ol' React

In which browsers you can reproduce the issue?

All

Describe the bug

When testing combobox with --detectLeaks using JEST, the test fails.

I have taken the unit test out of the repo directly, wrapped it in a mantineRender as per the documentation and ran a unit test with --detectLeaks - it appears that somewhere there is a leak within this component specifically.

Render Function

import React from 'react';
import { MantineProvider } from '@mantine/core';
import { render as testingLibraryRender } from '@testing-library/react';
// Import your theme object
import { theme } from 'libs/shared/src/theme/mantineTheme';

export function render(ui: React.ReactNode) {
  return testingLibraryRender(<>{ui}</>, {
    wrapper: ({ children }: { children: React.ReactNode }) => (
      <MantineProvider theme={theme}>{children}</MantineProvider>
    )
  });

}

Unit test

import React from 'react';
import { Combobox, TextInput, useCombobox } from '@mantine/core';
import { render } from './mantineRender';

function ComboTest() {
  const store = useCombobox({
    defaultOpened: true
  });

  return (
    <div style={{ padding: 40 }}>
      <Combobox store={store} withinPortal={false}>
        <Combobox.Target>
          <TextInput
            label="Test input"
            onFocus={() => store.openDropdown()}
            onBlur={() => store.closeDropdown()}
          />
        </Combobox.Target>
        <Combobox.Dropdown>
          <Combobox.Options aria-label="test">
            <Combobox.Option value="react">React</Combobox.Option>
            <Combobox.Option value="vue">Vue</Combobox.Option>
          </Combobox.Options>
        </Combobox.Dropdown>
      </Combobox>
    </div>
  );
}

describe('@mantine/core/Combobox', () => {
  it('renders with no leaks', async () => {
    const { container } = render(<ComboTest />);
    expect(container).toBeDefined();
  });
});

Jest reports:

EXPERIMENTAL FEATURE!
    Your test suite is leaking memory. Please ensure all references are cleaned.

    There is a number of things that can leak memory:
      - Async operations that have not finished (e.g. fs.readFile).
      - Timers not properly mocked (e.g. setInterval, setTimeout).
      - Keeping references to the global scope.

      at onResult (../../node_modules/jest/node_modules/@jest/core/build/TestScheduler.js:150:18)
          at Array.map (<anonymous>)

I tried utilising the <Text> component instead of just to make sure .. and it runs a test perfectly when calling --detectLeaks

If possible, include a link to a codesandbox with a minimal reproduction

No response

Possible fix

No response

Self-service

cyantree commented 4 months ago

Can you provide a complete reproduction repo? Then I'll give it a look.

webdistortion commented 4 months ago

https://github.com/webdistortion/mantine-memory-leak

^ - pull that repo and run npm run test - thanks very much for taking a look. This is using the NextJS quick start project.

cyantree commented 4 months ago

Thanks for the repo. I can reproduce the issue but I have trouble running a test that doesn't trigger it. Simply running an empty test triggers it.

For me it only disappears after commenting the following line in jest.setup.cjs:

// require('@testing-library/jest-dom');

After this an empty test runs without error.

However rendering a simple JSX element (without mantine) brings it back:

import {cleanup, render} from '@testing-library/react';

describe('Welcome component', () => {
  it('renders with no leaks', async () => {
    const { container, debug, unmount } = render(<b />);
  });
});

And this even seems to happen when render() isn't actually executed.

    if (false) {
      const { container, debug, unmount } = render(<b />);
    }

So as with the above require simply including the library results in the leak detector being triggered.

Is it the same for you?

Overall for me there's no way to tell whether something from mantine really is leaking. I think a more appropriate test would be rerunning the same test a number of times and checking whether the memory consumption increases steadily.

Also make sure to run cleanup() from @testing-library/react after each test: https://testing-library.com/docs/react-testing-library/api#cleanup

I hope that helps a bit.

webdistortion commented 4 months ago

This was absolutely not the case for me. What version of node are you on? I know there was memory leaks on early node versions with jest. Specifically I'm on 21 over here.

I was able to isolate the behaviour specifically to the comboBox.Dropdown - removing this part in the JSX passes the test.

     <Combobox.Dropdown>
      <Combobox.Options aria-label="test">
        <Combobox.Option value="react">React</Combobox.Option>
        <Combobox.Option value="vue">Vue</Combobox.Option>
      </Combobox.Options>
    </Combobox.Dropdown>

I'll do some further proof of concept work here if its required.

cyantree commented 4 months ago

I'm on the latest LTS on Windows - 20.12.2. I'll give 21/22 a try.

cyantree commented 4 months ago

Using Node 21 worked. Now I can dig deeper. :) Thanks for the hint!

cyantree commented 4 months ago

Hmm, it's really weird. For me the problem resolves as described by you by removing Combobox.Dropdown but also by only removing Combobox.Target.

Going deeper in Combobox.Dropdown removing ctx.floating from this line resolves it: https://github.com/mantinedev/mantine/blob/master/packages/%40mantine/core/src/components/Popover/PopoverDropdown/PopoverDropdown.tsx#L66

For Combobox.Target its removing ctx.reference in this line: https://github.com/mantinedev/mantine/blob/master/packages/%40mantine/core/src/components/Popover/PopoverTarget/PopoverTarget.tsx#L44

Going even deeper it's removing one of those lines that resolves it: https://github.com/mantinedev/mantine/blob/master/packages/%40mantine/core/src/components/Popover/Popover.tsx#L266 https://github.com/mantinedev/mantine/blob/master/packages/%40mantine/core/src/components/Popover/Popover.tsx#L274

And then it's into the depths of floating-ui where I haven't found anything yet.