Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

I am having non deterministic error after migrate to v12.4.0 #1572

Closed
CagriUysal opened this issue Mar 14, 2024 · 13 comments · Fixed by #1576
Closed

I am having non deterministic error after migrate to v12.4.0 #1572

CagriUysal opened this issue Mar 14, 2024 · 13 comments · Fixed by #1576
Labels
question Further information is requested

Comments

@CagriUysal
Copy link

Recently we have migrated react-native-testing-library from 9.1.0 to 12.4.0.
After the migration, we started receiving the following error when we ran the tests.

/builds/seller/mobile-core/microapps/gilgamesh-promotion/node_modules/@testing-library/react-native/build/screen.js:11
  throw new Error(SCREEN_ERROR);
  ^
Error: `render` method has not been called
    at Object.notImplemented (/builds/seller/mobile-core/microapps/gilgamesh-promotion/node_modules/@testing-library/react-native/src/screen.ts:7:9)
    at toJSON (/builds/seller/mobile-core/microapps/gilgamesh-promotion/node_modules/@testing-library/react-native/src/queries/make-queries.ts:94:23)
    at formatErrorMessage (/builds/seller/mobile-core/microapps/gilgamesh-promotion/node_modules/@testing-library/react-native/src/queries/make-queries.ts:106:19)
    at appendElementTreeToError (/builds/seller/mobile-core/microapps/gilgamesh-promotion/node_modules/@testing-library/react-native/src/queries/make-queries.ts:223:32)
    at Timeout.onTimeout [as _onTimeout] (/builds/seller/mobile-core/microapps/gilgamesh-promotion/node_modules/@testing-library/react-native/src/wait-for.ts:1[83](https://gitlab.trendyol.com/seller/mobile-core/microapps/gilgamesh-promotion/-/jobs/30696203#L83):24)
    at listOnTimeout (node:internal/timers:569:17)
    at processTimers (node:internal/timers:512:7)

The strange part is, that error occurs randomly in one of the test suits among 70. Failed suite is non-deterministic, error appears randomly in one of the suites. If I run tests for failed suit in isolation, they pass.

I've come across this StackOverflow question, it seems like other people came across this. I haven't opened this as a bug report since I can't reproduce it deterministically. Have you got any insight into why this might happen?

@CagriUysal CagriUysal added the question Further information is requested label Mar 14, 2024
@mdjastrzebski
Copy link
Member

Such error occurs when you call any of screen methods without calling render first. It's hard to give any recommendation without more details, but since you are mentioning non-deterministic behavior that would suggest that you have some race condition that is occurring in some case. Do you have any non-awaited async operation that happen before render call in failing tests?

@there
Copy link

there commented Mar 15, 2024

I also ran into the same error. For us it seemed to be related to using new Date() in some of our tests. I added this code to those tests which resolved the issue:

beforeEach(() => {
  jest
    .useFakeTimers({ doNotFake: ["nextTick", "setImmediate"] })
    .setSystemTime(new Date("2024-01-01"));
});

afterEach(() => {
  jest.useRealTimers();
});

Originally the tests were timing out when I tried to use jest.useFakeTimers() without the config, but I referenced this comment to resolve that issue:

@Marc-Cilliers
Copy link

We've just migrated from v11 to v12 and are experiencing the same thing. Out of ~4K tests, around 3-5 random tests fail on every test run, even though they pass in isolation.

I've tried the above solution of using fake timers as well as downgrading to v12.2.0 (earliest version that contains userEvent) but no luck.

@pierrezimmermannbam
Copy link
Collaborator

That's interesting, the stack trace you provided suggest the error comes from the fact that the component tree is now printed along with the error message when a findBy query fails. I guess what happens here is that this is somehow called after the screen has been unmounted? This should not happen though because waitFor won't reject before this is done. It could happen if you're not waiting for a findBy query somewhere and it rejects after your test is done and the cleanup has already occured? Could you look at the tests that fail and make sure every findBy call is correctly awaited?

@mdjastrzebski
Copy link
Member

@CagriUysal, @pierrezimmermannbam seems like a good trail. The stack trace and your analysis is indeed helpful.

I think we can improve the situation by:

  1. Make sure that find* queries access screen in a safe way
  2. Print warning/error to the console that your async queries have not been closed properly

I'll try to prepare a fix for that soon.

@mdjastrzebski
Copy link
Member

mdjastrzebski commented Mar 26, 2024

I've done some research and here's what I've found as most probable scenario:

  1. The presented error occurs when findBy* times out (not being able to find relevant element) after the test env has been cleaned up.
  2. The findBy* would throw either way, it should throw "Unable to find an element with ..." error, as it timed out, but it will throw "render method has not been called" instead.
  3. The root cause seems to be with the user test, as it did not await some findBy*/waitFor calls before the test ended
  4. The non-deterministic aspect might be that there is more than one async call being made, and this could cause race condition under which the awaited call sometimes "wins" and sometimes not.

What would be of great value is a repro scenario, even if it would be non-deterministic. Otherwise, we're just doing our best guess.

@pierrezimmermannbam, @CagriUysal, @there pls check improved error message in #1576.

@Marc-Cilliers
Copy link

I've just taken a look again. I managed to find one of the simpler tests that failed:

it('should show box with failure message', () => {
    const { getByText } = render(
      <MockThemeProvider>
        <NotificationBox type="failure" />
      </MockThemeProvider>
    );

    expect(getByText('Sorry, there’s a problem')).toBeDefined();
    expect(
      getByText('Process failed. Please try again later.')
    ).toBeDefined();
  });

with the stack trace:

`render` method has not been called

    at Object.notImplemented (../../node_modules/.pnpm/@testing-library+react-native@12.4.4_jest@29.7.0_react-native@0.72.5_react-test-renderer@18.2.0_react@18.2.0/node_modules/@testing-library/react-native/src/screen.ts:7:9)
    at toJSON (../../node_modules/.pnpm/@testing-library+react-native@12.4.4_jest@29.7.0_react-native@0.72.5_react-test-renderer@18.2.0_react@18.2.0/node_modules/@testing-library/react-native/src/queries/make-queries.ts:90:23)
    at formatErrorMessage (../../node_modules/.pnpm/@testing-library+react-native@12.4.4_jest@29.7.0_react-native@0.72.5_react-test-renderer@18.2.0_react@18.2.0/node_modules/@testing-library/react-native/src/queries/make-queries.ts:102:19)
    at appendElementTreeToError (../../node_modules/.pnpm/@testing-library+react-native@12.4.4_jest@29.7.0_react-native@0.72.5_react-test-renderer@18.2.0_react@18.2.0/node_modules/@testing-library/react-native/src/queries/make-queries.ts:199:32)
    at Timeout.onTimeout [as _onTimeout] (../../node_modules/.pnpm/@testing-library+react-native@12.4.4_jest@29.7.0_react-native@0.72.5_react-test-renderer@18.2.0_react@18.2.0/node_modules/@testing-library/react-native/src/wait-for.ts:177:24)

As far as I can tell, there's no floating promise present as getByText isn't asynchronous.
MockThemeProvider is also simple and has no async code;

export const MockThemeProvider = ({ children, theme = {} }: Props) => (
  <ThemeProvider theme={{ ...defaultTheme, ...theme }}>
    {children}
  </ThemeProvider>
);

Not sure if this helps or just muddies the waters further.
Let me know if there's anything I can provide from my side that might help

@pierrezimmermannbam
Copy link
Collaborator

@Marc-Cilliers ideally we'd need a reproduction repo to be able to debug properly, I can't tell what could be going wrong just looking at this test. You did mention that tests pass in isolation though, is that correct? If so it could indicate that the issue lies in another test and the error occurring during the teardown from that other test caused probably by a non resolved promise may affect this particular test. I would suggest looking for non awaited promises by using the eslint plugin testing library which has rules to make sure waitFor and findBy are correctly awaited

@Marc-Cilliers
Copy link

Marc-Cilliers commented Mar 27, 2024

@Marc-Cilliers ideally we'd need a reproduction repo to be able to debug properly, I can't tell what could be going wrong just looking at this test. You did mention that tests pass in isolation though, is that correct? If so it could indicate that the issue lies in another test and the error occurring during the teardown from that other test caused probably by a non resolved promise may affect this particular test. I would suggest looking for non awaited promises by using the eslint plugin testing library which has rules to make sure waitFor and findBy are correctly awaited

You're right! I added the eslint plugin, adjusted all tests where there were hanging promises and everything passing now! 🙌

I never suspected that a failure of 1 test might be the result of another test not correctly handling its promises.

@pierrezimmermannbam
Copy link
Collaborator

That's good news! That eslint plugin is very useful but not very frequently used I feel like, @mdjastrzebski maybe we should mention it on the readme? it is in the docs but it's probably missed by a lot of users

@mdjastrzebski
Copy link
Member

@pierrezimmermannbam Which ESLint plugin? The testing library one? https://callstack.github.io/react-native-testing-library/docs/eslint-plugin-testing-library

@pierrezimmermannbam
Copy link
Collaborator

Yeah exactly, there is a section in the docs about it, https://callstack.github.io/react-native-testing-library/docs/eslint-plugin-testing-library, the part about userEvent may not be accurate anymore by the way

@mdjastrzebski
Copy link
Member

@CagriUysal @there could you verify that installing eslint plugin and fixing indicated errors fixed this for you?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants