theKashey / react-focus-on

🎯 Solution for WAI ARIA compatible modal dialogs or full-screen tasks, you were looking for
MIT License
336 stars 14 forks source link

"Sidecar medium was not found" when running unit tests with jest #34

Closed pablitogo closed 1 year ago

pablitogo commented 4 years ago

Hello! I recently added this library to add accessibility features to a modal dialog component (and it works like a charm, thanks!) but I'm having some issues with the tests that use that component. The tests are now failing with the error Sidecar medium was not found.

Using react-testing-library and jest. The test also gives some warnings about updates to sidecar not being wrapped inside an act function, which they are so that seems strange too

Warning: An update to Sidecar inside a test was not wrapped in act(...).

When testing, code that causes React state updates should be wrapped into act(...):

      act(() => {
        /* fire events that update state */
      });
      /* assert on the output */

Unfortunately the error messages aren't very helpful so I would appreciate some advice

Thanks

theKashey commented 4 years ago

It's working like this:

In other words - focus-on is asking for Effect, sidecar is importing it and expecting that Effect would provide itself, but to record about it found.

In other other words - I know where things are breaking, but have to idea why. Our own tests are written in Jest, and there is no problem with it.

theKashey commented 4 years ago

I am not sure what is not working, so I am going to add some debug information and make the error more readable.

theKashey commented 4 years ago

Could you shed some light - are you using react-focus-on/UI/sidecar or just react-focus-on? They do work a bit differently, and the problem with act is bound to the first case.

theKashey commented 4 years ago

In the case of UI/sidecar - there is more that one way to solve the problem, I am not sure which one to go. If it's possible - try to switch to react-focus-on, which is synchronous non-code-splittable version, without any useEffects inside - the override could be done using jest rewrites, just rewrite react-focus-on/UI to react-focus-on - they are 100% compatible, the difference is in composition.

pablitogo commented 4 years ago

Hi @theKashey

Thanks for looking into this.

Could you shed some light - are you using react-focus-on/UI/sidecar or just react-focus-on? They do work a bit differently, and the problem with act is bound to the first case. I have tried both approaches but id didn't make a difference

It's also worth mentioning that the test I'm running is more like an integration test so I'm not mocking the react-focus-on package or any of its dependencies. Have you tried if you get the same error on integration tests?

theKashey commented 4 years ago

So that's the problem - I do have integration tests among focus-on itself, as well as in other projects, and they seems to be ok.

I am not sure what's happening :(

theKashey commented 4 years ago

Any luck so far?

pablitogo commented 4 years ago

Hey @theKashey , sorry I just came back from holiday. Happy new year! No luck unfortunately, I couldn't really invest more time in troubleshooting the issue so I went for an alternative implementation explicitly using react-scroll-locky and react-focus-lock for the modal implementation

theKashey commented 4 years ago

Keep me in touch. You are the only one who are experiencing(or at least reported) this problem, so you are the only chance to fix it as well.

sehoven commented 4 years ago

Hi @theKashey , I experienced this issue as well using react-focus-lock/UI (which I assume is the same issue here since react-focus-on is also using that library).

Warning: An update to Sidecar inside a test was not wrapped in act(...).

    When testing, code that causes React state updates should be wrapped into act(...):

    act(() => {
      /* fire events that update state */
    });
    /* assert on the output */

Here's an example of what my code looked like:

MyComponent.jsx (this gets the error when I run the test below)

import FocusLockUI from 'react-focus-lock/UI';
import { sidecar } from 'use-sidecar';
import useSmallScreen from './hooks/useSmallScreen';

const MyComponent = () => {
  const isSmallScreen = useSmallScreen();
  const FocusLockSidecar = sidecar(() => import('react-focus-lock/sidecar'));
  return (
    <FocusLockUI disabled={!isSmallScreen} sideCar={FocusLockSidecar}>
      // more stuff
    </FocusLockUI>
  )
}

MyComponent.test.jsx (using react-testing-library and jest)

  test('matches snapshot with small screen', () => {
    mockUseSmallScreen.mockReturnValue(true);

    const { asFragment } = render(<MyComponent />);
    expect(asFragment()).toMatchSnapshot();
  });

I tried using the non code splittable version (react-focus-lock) instead, and I don't get the error when I run the same test. MyComponent.jsx (no error)

import FocusLock from 'react-focus-lock';
import useSmallScreen from './hooks/useSmallScreen';

const MyComponent = () => {
  const isSmallScreen = useSmallScreen();
  return (
    <FocusLock disabled={!isSmallScreen}>
      // more stuff
    </FocusLock>
  )
}

It makes sense why the error is happening using the code splittable version, I'm just not sure how to write my test to fix it. For now I'm using react-focus-lock instead of the code splittable version, but hopefully this helps if you wanted to look into this error further or if anyone else is trying to debug this error and finds this issue like I did.

theKashey commented 4 years ago
  1. Please move const FocusLockSidecar = sidecar(() => import('react-focus-lock/sidecar')); out of component. It should be created once and for all.

  2. You can import FocusLock from 'react-focus-lock'; in tests to pre-require sidecar. Not 100% sure it removes the warning, but it should.

dylanjeffers commented 4 years ago

@pablitogo @sehoven if it helps, this worked like a charm for me:

in jest.config.js:

moduleNameMapper: {
  ...
  "react-focus-on/UI": "react-focus-on",
}

@theKashey also tried the two options you posted above (option 1 should be done regardless :)), but didn't end up working for me :/

theKashey commented 4 years ago

Your solution is actually very, very good. I am a huge fan of dependency replacement and just 😃 that it works for this case.

However look like a bit more testing from my side are required. Option 2 should really work (it's in sidecar own tests by fact)

brandon-pereira commented 3 years ago

Aha, yes. I got this error as well because we were doing dependency replacement. One repo was on v3.5.0 and one repo was on v3.3.0. Once I aliased everything to use the same instance, it worked like a charm :)

theKashey commented 3 years ago

https://github.com/atlassian/yarn-deduplicate might help with resolving such "version overlap" issues, as well as I am working on porting https://github.com/atlassian-labs/webpack-deduplication-plugin to nodejs/jest env to automate aliasing of such sort.

stale[bot] commented 1 year ago

This issue has been marked as "stale" because there has been no activity for 2 months. If you have any new information or would like to continue the discussion, please feel free to do so. If this issue got buried among other tasks, maybe this message will reignite the conversation. Otherwise, this issue will be closed in 7 days. Thank you for your contributions so far.