mockClear does not reset the environment properly

[artola]

createMockEnvironment returns an instance of environment with mockClear, but this method does not fully clean the internals. For example, we would like to create only 1 environment and use mockClear between tests to reset the internals in place of running createMockEnvironment in each of them, in this way we could move the test setup to jest.env.js loaded on setupFilesAfterEnv, in some way similar to this example:

const environment = createMockEnvironment();

const Provider: React.FC = ({children}) => (
  <RelayEnvironmentProvider environment={environment}>
    <ErrorBoundary>
      <Suspense fallback={null}>{children}</Suspense>
    </ErrorBoundary>
  </RelayEnvironmentProvider>
);

const Dummy: React.VFC<{fetchKey?: string | number}> = ({fetchKey}) => {
  const data = useLazyLoadQuery(
    graphql`
      query dummyQuery {
        foo
      }
    `,
    {},
    {fetchKey},
  );

  return (
    <div dangerouslySetInnerHTML={{__html: JSON.stringify(data, null, 2)}} />
  );
};

describe(() => {
  afterEach(() => {
    environment.mockClear();
  });

  it('loading', () => {
    render(<Dummy />);
    // none resolve / reject 
    expect(...);
  });

  it('error', () => {
    render(<Dummy />);
    environment.mock.rejectMostRecentOperation(new Error('oops'));
    expect(...);
  });

  it('data', () => {
    render(<Dummy />);
    environment.mock.resolveMostRecentOperation({foo: 123})
    expect(...);
  });
});

At present if we run each of the tests above individually, they work, but if we try them altogether, then they fail. For example with:

Invariant Violation: RelayModernMockEnvironment: There are no pending operations in the list

The former error is because the environment holds some module state for requestCachesByEnvironment.

And for more complex scenarios where multiple tests are fired in a row, we can hit 2 other caches that are not cleared in mockClear. Also we must clear the store between tests as already reported and PR.

To sum up, just adding a few lines in the mockClear we could use the same instance all the time. Here a snippet with some methods that "properly" exposed in the API will make it work:

https://github.com/facebook/relay/blob/741741acc84cc03a3c7cba3014b5c5bf210f78d9/packages/relay-test-utils/RelayModernMockEnvironment.js#L534

import {getFragmentResourceForEnvironment} from 'react-relay/lib/relay-hooks/FragmentResource'; // <== not exposed in barrel file
import {getQueryResourceForEnvironment} from 'react-relay/lib/relay-hooks/QueryResource'; // <== not exposed in barrel file
import {getRequestCache} from 'relay-runtime/lib/query/fetchqueryInternal';  // <== not exported by the module

...
  function createMockEnvironment(
...
  const qr = getQueryResourceForEnvironment(environment);
  const fr = getFragmentResourceForEnvironment(environment);
  const rc = getRequestCache(environment);

    qr._cache.clear();
    fr._cache.clear();
    rc.clear();

    // @see https://github.com/facebook/relay/pull/3687
    environment.getStore().getSource().clear();

@kassens @alunyov @sibelius Do you foresee something against it? Please advise.

[alunyov]

I think it will be very hard to keep track of the different caches we have in Relay (we may add more in the future, and change the current) and keep them in sync with createMockEnvironment.

The correct approach is just to create a new environment for every test.

[alunyov]

This brings an interesting point: I almost feel like this API mockClear should be deprecated. Its existence makes it feel like it is possible to just completely reset the internal state of the environment, which is not what it's doing right now. If we want developers to use the correct pattern: create a new environment for test, we should not have mockClear.

I think we were using it for some internal tests, at some point. But maybe we need to re-evaluate the usage of it.

[artola]

@alunyov I agree mockClear should fully work, or be removed (at least documented) to avoid misunderstandings.

Even, more picky, it is not the best as it expect to run in Jest environment, no other test framework (allowing by configuration), but even not listed in the dependencies of any kind.

https://github.com/facebook/relay/blob/be5e9017550e66bb4c061ce0ff6e7be461cb7a07/packages/relay-test-utils/RelayModernMockEnvironment.js#L54

First time that I saw such code feel it "wrong". In general we use Jest as it is mainstream, but just a week ago I was doing some tests on Mocha to compare some issues.

And I found strange this use of jest.fn (surely to make it easier in some use cases) but very rare that we need to wrap in act the calls to resolveMostRecentOperation where it could be handy, similar to RTL fireEvent.click (built-in).