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

Document better the details of aync methods #1012

Open
adamhenson opened this issue Feb 8, 2022 · 7 comments
Open

Document better the details of aync methods #1012

adamhenson opened this issue Feb 8, 2022 · 7 comments

Comments

@adamhenson
Copy link

adamhenson commented Feb 8, 2022

Is your feature request related to a problem? Please describe.

In the example seen at the bottom of this issue, I get flaky results. The assertion passes sometimes and fails sometimes. This comes from a lack of understanding by me of how async methods work. I've Googled the heck out of it and read the docs linked below a bunch of times. I also looked through the source code, but I found it to be difficult to understand from someone who hasn't worked in this project.

Describe the solution you'd like

Ultimately, I think there are a couple of waitFor options that should be documented in more detail. I'd like the below questions to be answered.

  • What is the behavior of interval option?
  • What is the behavior of the timeout option?

I think this should be covered in detail here: https://testing-library.com/docs/dom-testing-library/api-async/

Describe alternatives you've considered A clear and concise description of
any alternative solutions or features you've considered.

NA

Additional context Add any other context or screenshots about the feature
request here.

Example code that has flaky results. I don't think the details of useDataFetch is important here, but let me know if I can answer any questions about it.

it('displays fetched data', async () => {
  const increment = createIncrement();
  const MyComponent = () => {
    const { data } = useDataFetch(async () => {
      await sleep(1);
      return increment();
    });
    return <div>times fetch called: {data}</div>;
  };
  
  render(<MyComponent />);
  const element = await screen.findByText('times fetch called: 2');
  expect(element).toBeInTheDocument();
});
@eps1lon
Copy link
Member

eps1lon commented Feb 8, 2022

Thanks for the detailed explanation.

Could you clarify how understanding interval and timeout helped you fix the flakyness? And please clarify what "flaky results" specifically means (fails with what error(s)?).

@adamhenson
Copy link
Author

adamhenson commented Feb 8, 2022

Hi @eps1lon:

And please clarify what "flaky results" specifically means (fails with what error(s)?).

To clarify what I mean by "flaky results" - the assertion above would fail sometimes and pass sometimes with no change to the code under test. The failure would be along the lines of the element not found. The log showed an element with content like this times fetch called: 1, which tells me that findByText doesn't know what to wait for and didn't wait long enough for the correct content to display times fetch called: 2 (since there isn't any "real" side effect code, the wait time should literally be 1ms + time it takes to render).

Could you clarify how understanding interval and timeout helped you fix the flakyness?

The reason I opened this ticket is because I failed to get an understanding of interval and timeout. The docs don't describe why one might want to set these options and what they do. I blindly set timeout to 2000, but I have no idea if it will cure the flakiness and why. So, I'd like to know if I can utilize those options to fix my issue... I just don't know exactly what they're used for.

I'd personally like a better understanding of those options, but I think they should also be provided in the documentation per this issue.

@eps1lon
Copy link
Member

eps1lon commented Feb 8, 2022

the assertion above would fail sometimes and pass sometimes with no change to the code under test.

I understand what flaky means. The question is what specific error(s) you see. That's really important here. A vague "sometimes it errors, sometimes it doesn't" will not help us identify the issue.

I'd personally like a better understanding of those options, but I think they should also be provided in the documentation per this issue

Yeah I agree that we assume too much knowledge here. Interval is slightly more described but we should be explicit what they do:
interval specifies the interval at which we check the condition (e.g. if the element exists).
If the condition is not true after timeout we fail.

We should document this more clearly with concrete examples.

@adamhenson
Copy link
Author

Thanks! So, ultimately timeout negates when to end and interval checks occur before the final timeout. I thought I provided enough details about the error:

The failure would be along the lines of the element not found. The log showed an element with content like this times fetch called: 1

@adamhenson
Copy link
Author

@eps1lon here is the full error:

 PASS  src/useLazyDataFetch.test.tsx
 FAIL  src/useDataFetch.test.tsx
  ● useDataFetch › triggers a refetch when explicitly called

    TestingLibraryElementError: Unable to find an element with the text: times fetch called: 2. This could be because the text is broken up by multiple elements. In this case, you can provide a function for your text matcher to make your matcher more flexible.

    <body>
      <div>
        <div>
          times fetch called: 
          1
        </div>
      </div>
    </body>

    <body>
      <div>
        <div>
          times fetch called: 
          1
        </div>
      </div>
    </body>Error: Unable to find an element with the text: times fetch called: 2. This could be because the text is broken up by multiple elements. In this case, you can provide a function for your text matcher to make your matcher more flexible.

    <body>
      <div>
        <div>
          times fetch called: 
          1
        </div>
      </div>
    </body>

      74 |         render(<MyComponent />);
      75 |
    > 76 |         const element = await screen.findByText('times fetch called: 2');
         |                                      ^
      77 |
      78 |         expect(element).toBeInTheDocument();
      79 |     });

      at waitForWrapper (../../../node_modules/@testing-library/dom/dist/wait-for.js:173:27)
      at ../../../node_modules/@testing-library/dom/dist/query-helpers.js:101:33
      at useDataFetch.test.tsx:76:38
      at useDataFetch.test.tsx:27:71
      at Object.<anonymous>.__awaiter (useDataFetch.test.tsx:23:12)
      at Object.<anonymous> (useDataFetch.test.tsx:59:64)

Test Suites: 1 failed, 1 passed, 2 total
Tests:       1 failed, 8 passed, 9 total
Snapshots:   0 total
Time:        3.943 s

@adamhenson
Copy link
Author

adamhenson commented Feb 8, 2022

useDataFetch FWIW (sorry, they use 4 spaces per tab at my job):

import { Reducer, useEffect, useState, useReducer } from 'react';
import { DataFetchStateType, DataFetchActionType } from './types';
import reducer from './reducer';

export default function useDataFetch<DataType>(
    fetchData: (...args: any[]) => Promise<DataType>,
    ...args: any[]
) {
    const [nonce, setNonce] = useState(Date.now());
    const [state, dispatch] = useReducer<
        Reducer<DataFetchStateType<DataType>, DataFetchActionType<DataType>>
    >(reducer, {
        data: null,
        error: null,
        loading: true,
    });

    useEffect(() => {
        let cancel = false;

        const callDispatches = async () => {
            dispatch({ type: 'get' });
            try {
                const data = await fetchData(...args);
                if (!cancel) {
                    dispatch({ type: 'success', payload: { data } });
                }
            } catch (error) {
                dispatch({ type: 'error', payload: { error: error as Error } });
            }
        };

        callDispatches();

        return () => {
            cancel = true;
        };
    }, [nonce, ...args]);

    const refetch = () => {
        setNonce(Date.now());
    };

    return { ...state, refetch };
}

@adamhenson
Copy link
Author

adamhenson commented Feb 8, 2022

So, I guess my inclination to increase timeout was correct... although not sure why the default 1000 value wouldn't work (or at least I think that is the default). What I was missing from the docs is that interval is responsible for "checking" before the final timeout.

Perhaps I could open an PR.

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

No branches or pull requests

2 participants