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

Move spying inside proxy #28

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

green-arrow
Copy link

@green-arrow green-arrow commented Dec 22, 2020

There are 3 failing tests, and it seems to be around using the sandbox mode. I'm a bit out of my depth when it gets to what's happening with the sandbox, but for some reason it seems the fetchHandler isn't even being called when I debug a specific test (like "exposes calls property" inside the jest-built-ins spec).

fixes #26

@wheresrhys
Copy link
Owner

Thanks for the fix. I'm gonna try to figure out exactly why it was failing before merging in order to lessen the risk of unintended side effects. I also want to add jobs in CI that test against jest 25 & 26. Should hopefully have some time to do so today.

@wheresrhys
Copy link
Owner

I've added jest@26 to the build pipeline and the test spass on master. Are you able to set up a reduced test case illustrating the situation when things are breaking for you. Pasting in a comment here is fine

@green-arrow
Copy link
Author

Here is the gist of the test case that is failing:

    it('should handle address input changing', () => {
      const handleAddressChange = jest.fn();
      const { getByLabelText } = render(
        <AddressAutocomplete
          name={name}
          label={label}
          address={''}
          handleAddressChange={handleAddressChange}
          {...i18nProps}
        />
      );
      const input = getByLabelText(label, { selector: 'input' });
      const value = '123';
      const url = `${TEST_URL}/actions/addressPredictions?filter[address]=${value}`;

      const addressMock = fetchMock.mock(
        url,
        { status: 200, body: [] },
        {
          method: 'GET',
        }
      );

      fireEvent.focus(input);
      fireEvent.change(input, { target: { value } });
      jest.runAllTimers();

      expect(addressMock).toHaveBeenCalled();
      expect(handleAddressChange).toHaveBeenCalledTimes(1);
      expect(handleAddressChange).toHaveBeenCalledWith({ address: value });
    });

The actual component implementation just debounces the search value and then executes a redux dispatch which in turn triggers the fetch. Not really much special going on. Unless it's something weird with using the timers and delaying the fetch call.

@green-arrow
Copy link
Author

@wheresrhys - could it potentially be a node version issue? I'm running v14.

@wheresrhys
Copy link
Owner

wheresrhys commented Jan 4, 2021

Is the test running in the browser? Or in a browser-like environment (e.g. JSDom) and mocking the fetch global? .sandbox() isn't meant to be used in that case.

I'll probably need to see the failing test + all the test runner set up in order to properly debug

@green-arrow
Copy link
Author

green-arrow commented Jan 4, 2021

These tests are running as part of a create-react-app, latest version of react-scripts and its jest configuration, along with jest 26, so using JSDom.

I only mentioned the sandbox because I thought it was relevant to some failing tests I had locally, but I now see that all of the test cases use the sandbox method.

There are a couple other cases in our codebase that do not do any debouncing or timer related business, and those also have the same issue, so I don't think it's anything there like I suggested earlier. These are simpler functions that just call the browser fetch, similar to most of the test cases you already have.

Edit: I'm not using sandbox anywhere in my code, just doing import fetchMock from 'fetch-mock-jest'; in the test and using the browser fetch like normal in app code.

@wheresrhys
Copy link
Owner

I don't really have at the moment to set up a full environment to replicate. If you could create a minimal test case repo I could clone (or are able to share an existing repo with me) that'd be a great help

@green-arrow
Copy link
Author

Sorry for the delay in getting back with you, I'm working through a separate upgrade issue with another library since I found a workaround for this. I should hopefully have that wrapped up today / tomorrow, then I plan on putting together a codesandbox to try and replicate the issue.

@SpencerKaiser
Copy link

@wheresrhys @green-arrow don't forget about this one! Based on your conversation, it sounds like y'all are close! 🤞

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

Successfully merging this pull request may close these issues.

fetchMock.mock.calls never updates (possibly jest 26)
3 participants