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

useEffect not called when the component is shallow renderered #2086

Open
2 of 13 tasks
jlandic opened this issue Apr 8, 2019 · 68 comments
Open
2 of 13 tasks

useEffect not called when the component is shallow renderered #2086

jlandic opened this issue Apr 8, 2019 · 68 comments
Projects

Comments

@jlandic
Copy link

jlandic commented Apr 8, 2019

Current behavior

useEffect fonction does not seem to be executed when the component is rendered with shallow.

Given the following simple component using useEffect hook to call a method given as a prop:

import React, { useEffect } from 'react';

export const MyComponent = ({ callback }) => {
  useEffect(() => {
    callback('Say something');
  });

  return <div>Hello world!</div>;
};

And the following tests checking that the callback function is indeed called accordingly:

import React from 'react';
import { shallow, mount } from 'enzyme';
import { MyComponent } from '../MyComponent';

describe('MyComponent', () => {
  describe('shallow render', () => {
    it('should call callback when shallow rendered', () => {
      const callback = jest.fn();
      shallow(<MyComponent callback={callback} />);
      expect(callback).toHaveBeenCalled();
    });
  });

  describe('mount', () => {
    it('should call callback when mounted', () => {
      const callback = jest.fn();
      mount(<MyComponent callback={callback} />);
      expect(callback).toHaveBeenCalled();
    });
  });
});

We observe that the method is called as expected when "mounted", but not when using a shallow renderer:

 FAIL  src/tests/MyComponent.test.js
  MyComponent
    shallow render
      ✕ should call callback when shallow rendered (12ms)
    mount
      ✓ should call callback when mounted (24ms)

  ● MyComponent › shallow render › should call callback when shallow rendered

    expect(jest.fn()).toHaveBeenCalled()

    Expected mock function to have been called, but it was not called.

       8 |       const callback = jest.fn();
       9 |       shallow(<MyComponent callback={callback} />);
    > 10 |       expect(callback).toHaveBeenCalled();
         |                        ^
      11 |     });
      12 |   });
      13 |

      at Object.toHaveBeenCalled (src/tests/MyComponent.test.js:10:24)

You may find this reproducible example here.

Expected behavior

When using shallow to render the components, the function passed to useEffect should be executed, as it is when using mount.

Your environment

API

  • shallow
  • mount
  • render

Version

library version
enzyme 3.9.0
react 16.8.6
react-dom 16.8.6
react-test-renderer 16.8.6
adapter (below)

Adapter

  • enzyme-adapter-react-16
  • enzyme-adapter-react-16.3
  • enzyme-adapter-react-16.2
  • enzyme-adapter-react-16.1
  • enzyme-adapter-react-15
  • enzyme-adapter-react-15.4
  • enzyme-adapter-react-14
  • enzyme-adapter-react-13
  • enzyme-adapter-react-helper
  • others ( )
@vinicius33
Copy link

Same here. Do you have any idea when this is going to be supported? 🙏
Thanks,

@ljharb
Copy link
Member

ljharb commented Apr 13, 2019

Hopefully in the next version of enzyme.

@ljharb ljharb added this to v16.8+: Hooks in React 16 Apr 13, 2019
@bdwain
Copy link
Contributor

bdwain commented Apr 30, 2019

Was there a plan to support that? I made an issue in react to support this from the shallow renderer with an option. I figured that would be the only way to do it without trying to mock out useEffect or something

@ljharb
Copy link
Member

ljharb commented May 1, 2019

ah, fair enough - you're right that it still won't work in shallow, and that that issue, or mocking it out in enzyme, would be the only way to make it work.

@mangel0111
Copy link

Is there a plan or PR to fix this? Makes hard include hoos/useEffect if we can't test it

@bdwain
Copy link
Contributor

bdwain commented May 2, 2019

@mangel0111 see the previous 2 comments. we're waiting to see if we can add a feature to react which will make it easier to fix this

@tomasro27
Copy link

Having the same issue here. Is there any alternative to creating a snapshot that runs the useeffect hooks?

@MM263

This comment has been minimized.

@gonzofish

This comment has been minimized.

@MM263

This comment has been minimized.

@insidewhy
Copy link

insidewhy commented Jul 21, 2019

It looks like the render and update calls just need to be called inside an act() callback and this will flush the effects, whether using shallow rendering or not. I have this working in a hacked together enzyme now. EDIT: I made a mistake, implemented the feature below though.

@bdwain
Copy link
Contributor

bdwain commented Jul 21, 2019 via email

@insidewhy
Copy link

@bdwain Sorry I had a bunch of changes to ReactShallowRenderer which I was supposed to remove before adding the act() wrapper. Actually the act() wrapper changed nothing. It's not that hard to make the change but react maintainers don't seem to be interested at the moment.

@bdwain
Copy link
Contributor

bdwain commented Jul 21, 2019

yea i wonder if they would be more interested if it was completed already. If you got it all working maybe post that in a PR and see what they say?

@insidewhy
Copy link

@bdwain My PR is here: facebook/react#16168 would be really grateful for your feedback.

@insidewhy
Copy link

insidewhy commented Jul 25, 2019

BTW it's possible to get this working from enzyme without having to touch react, but you have to override _createDispatcher in the shallow renderer to call the original version and patch useEffect and useLayoutEffect with my versions from the PR above and return the patched dispatcher. Then after enzyme calls render you just add in my effect calling code from the PR above.

Would be nicer if the code was in React's shallow renderer though.

@insidewhy
Copy link

I needed this feature like... 5 months ago. Which is about the length of time the react maintainers have been ignoring the issue for. If they don't respond to my PR in another few weeks could I maybe do the private method override thing? I would make enzyme's code test for the existence of the private method first so as not to break if the internals change. Then when (or more like if) react accept the PR or code then it could just be removed. Although it is super hacky :( In my own project I switched from enzyme to the test renderer and do the hack now so I can at least test my effects, but I'd much rather be using enzyme.

@ljharb
Copy link
Member

ljharb commented Aug 21, 2019

@ohjames as long as your tests here are good, that is totally fine.

@insidewhy
Copy link

@ljharb Cool, I've written the code now. Having trouble testing it. After running the steps listed in CONTRIBUTORS.md I get the following after npm run react 16:

james@lapchil enzyme % npm run react 16                                                                         
                                                                                                                
> enzyme@0.0.1 react /home/james/projects/enzyme                                                                
> sh install-relevant-react.sh "16"                                                                             
                                                                                                                
installing React 16                                                                                             
                                                                                                                
> enzyme@0.0.1 env /home/james/projects/enzyme                                                                  
> babel-node ./env.js "16"                                                                                      
                                                                                                                
rimraf ./node_modules/react                                                                                     
rimraf ./node_modules/react-dom                                                                                 
rimraf ./node_modules/react-addons-test-utils                                                                   
rimraf ./node_modules/react-test-renderer                                                                       
rimraf ./node_modules/create-react-class                                                                        
rimraf node_modules/.bin/npm 
rimraf node_modules/.bin/npm.cmd
npm i
npm WARN checkPermissions Missing write access to /usr/lib/node_modules/npm/node_modules

Hmmm... not sure why it wants to install things to system owned directories. Don't really want to contaminate my system :(

@insidewhy
Copy link

insidewhy commented Aug 21, 2019

Okay... so I decided to run it in a container (I chose node:11) because I didn't want to run things as root just to get this working (and have lost all interest in trying lerna which I thought previously might be a good idea). But get 11 failing tests with a totally clean codebase. Hmm... enzyme's build system is intense and scary.

Here's some examples of failing tests:

  1) Adapter provides node displayNames supports Profiler:                                                      
     AssertionError: expected null to equal 'Profiler'                                                          
      at Context.<anonymous> (packages/enzyme-test-suite/test/Adapter-spec.jsx:1048:47)                         
      at processImmediate (internal/timers.js:443:21)

  2) Adapter provides node displayNames supports ConcurrentMode:                                                
     AssertionError: expected null to equal 'ConcurrentMode'                                                    
      at Context.<anonymous> (packages/enzyme-test-suite/test/Adapter-spec.jsx:1052:53)                         
      at processImmediate (internal/timers.js:443:21)

  10) (uses jsdom) mount Profiler wrapped: with console throws: mounts without complaint:                       
     AssertionError: expected [Function] to not throw an error but 'EvalError: Warning: React.createElement: typ
e is invalid -- expected a string (for built-in components) or a class/function (for composite components) but g
ot: %s.%s%s' was thrown                                                                                         
      at Context.<anonymous> (packages/enzyme-test-suite/test/ReactWrapper-spec.jsx:865:9)                      
      at processImmediate (internal/timers.js:443:21)                                                           
                                                                                                                
  11) (uses jsdom) mount Profiler wrapped: with console throws: "after each" hook: afterEachWithOverrides:
     Uncaught EvalError: The above error occurred in the <SomeComponent> component:                             
    in SomeComponent (created by WrapperComponent)                                                              
    in WrapperComponent

Now I kinda feel like giving up.

@ljharb

This comment has been minimized.

@insidewhy

This comment has been minimized.

@LeonardPurschke

This comment has been minimized.

@insidewhy

This comment has been minimized.

@ljharb

This comment has been minimized.

@kevinding0218
Copy link

Hey guys, if I would like to test my mock of useEffect toHaveBeenCalled, how would I use the way as introduced above?

jest.mock('react', () => ({
  ...jest.requireActual('react'),
  useEffect: (f) => f(),
}));

I have tried this as well but it seems useEffect is not able to be mocked for real

const mockUseEffect = jest.fn();
jest.spyOn(React, 'useEffect').mockImplementation(() => mockUseEffect);

...
expect(mockUseEffect).toHaveBeenCalled() // failed as seems my actual useEffect is being called rather than my mocked function

Any guide on how to test the useEffect with mocked as toHaveBeenCalled() or not.toHaveBeenCalled()?

@stvpalumbo
Copy link

@kevinding0218 I suggest you observe the effects wired up inside your useEffect instead of checking whether useEffect is called. In the approach you are considering, you are mocking the useEffect implementation to always execute the registered effect. This means that on every render the useEffect will trigger.

But to answer your question:

jest.mock('react', () => ({
  __esModule: true, // maybe required?
  ...jest.requireActual('react'),
  useEffect: jest.fn(f => f()),
}));
import React, { useEffect } from 'react';

// before each reset the mock

// in the test
expect(useEffect).toHaveBeenCalled();

@nathanmrtns
Copy link

I got it to work using mount instead of shallow

@shiraze
Copy link

shiraze commented Aug 23, 2020

Seriously guys, just use mount() instead of shallow(). Changes are minimal, and you can still mock sub-components to stop full tree-loading.
As an example of how easy the switch is, I had code as follows:

    shallow(<RequireLogon {...props} />).setProps({
      logonExpiresAt: "same date",
    });

that was changed to the following when a class component was converted to a functional component, and logic from lifecycle events (e.g. componentDidUpdate) was moved to useEffect:

    const wrapper = mount(<RequireLogon {...props} />);
    wrapper
      .setProps({
        logonExpiresAt: "same date",
      })
      .update();

@davidmfoley
Copy link

Deleting my previous comments since they are a bit confusing; to summarize:

This adapter supports useEffect and useLayoutEffect with shallow:
https://www.npmjs.com/package/enzyme-adapter-react-16-with-shallow-effects

It does so by monkey-patching the dispatcher's handlers. It also uses react-shallow-renderer rather than react-test-renderer/shallow, but that is a less substantive change that should not affect usage.

I could adapt these changes for inclusion in the official adapter and open a PR.... but note:

  • this would be a behavior change that could break tests unexpectedly.
  • this relies on not-explicitly-public implementation details of react-shallow-renderer

@Gyllsdorff
Copy link

Seriously guys, just use mount() instead of shallow(). Changes are minimal, and you can still mock sub-components to stop full tree-loading.

Lets say I have a component X that renders a tree like this :

<>
    <Y bar={baz} />
    <Z shouldFetch>Hello</Z>
    <p>Lorem ipsum</p>
    <button onClick={onClick}>Click me</button>
</>

If we use mount in the test cases involving X then all of those tests have to know exactly which children X uses and know enough to understand that the child Y is a simple component that we don't have to mock and that the Z child makes API call so we should mock that component. We also have to keep track of how many tests cases we have to be updated if we add a useEffect or complex logic to Y which makes ticket estimations harder since changing a component that is used in multiple places might result in changes for 50+ test cases.

It is certainly possible to do this in smaller components, components with few test cases, or components with many test cases where each root can be setup by a shared function, but that is not the case for many of our tests.

Replacing shallow with mount is a major increase in test maintenance workload and increases the cost of making changes in the tested components, shallow is also much faster than mount so this also slows down the code/test feedback loop.

@davidmfoley
Copy link

Perhaps opinions on testing approaches could be moved to forums better suited to discussion?

@aarowman
Copy link

aarowman commented Sep 9, 2020

+1 to last comment from @Gyllsdorff . Yes, it may be possible to use mount instead with proper mocks, but that adds additional overhead and complication to the tests. It also may have other side effects. The entire point of using shallow is to provide a single reference to the component without going deeper. This issue is one I've had my eye on for a while and am really hoping for a fix!

For now, I guess we'll keep with the more complicated way. At least we have a workaround.
Just because there is a workaround, that doesn't mean it's best.

@anosov-otg
Copy link

jest.mock('react', () => ({
  ...jest.requireActual('react'),
  useEffect: (f) => f(),
}));

Works fine, but keep in mind that it will run for every effect and on every render even if their dependencies are still the same. There's a pretty ugly workaround, but it works:

let mockLastDeps = {};
jest.mock("react", () => ({
  ...jest.requireActual("react"),
  useEffect: (f, deps) => {
    const effect = new Error().stack.split("\n")[2]; // <-- to find fileName and lineNumber, since we can't identify by `f`
    if (!mockLastDeps[effect] || deps.some((dep, i) => dep !== mockLastDeps[effect][i])) {
      f();
      mockLastDeps[effect] = deps;
    }
  },
}));

@isaacaskew
Copy link

Ran into a use case for this; ended up going with jest-react-hooks-shallow like was linked above to work around it. Definitely interested in this support coming by default from enzyme!

@davidmfoley
Copy link

davidmfoley commented Oct 26, 2020

I spent a fair bit of time working on this and even made an enzyme adapter with hook support (see above), but in the end I decided to create a replacement for enzyme shallow that supports the full set of hooks and is self-contained: https://www.npmjs.com/package/isolate-components

I think it should support most of what shallow does except for dive(), albeit with a different/simpler interface. Also if you want to to test your hooks directly, without components, I made this: https://www.npmjs.com/package/isolate-hooks

@ljharb
Copy link
Member

ljharb commented Oct 26, 2020

@davidmfoley interesting! could you help me understand the difference between isolate-components and shallow (if it had full hook support)? In other words, why could your package not be used or adapter into improving shallow itself?

@davidmfoley
Copy link

davidmfoley commented Oct 26, 2020

@ljharb

The big difference in implementation is that isolate-components does not use an external renderer. For the most part, the features are similar to enzyme shallow with only react 16+ support, but there are some other cases that I want to support in future versions that don't really fit into enzyme (the big ones: 1) ability to "inline" a subcomponent to support cases where you refactor a component into smaller compnents without breaking tests and 2) "mount" without a virtual DOM).

I did start down the path of building a hook-compatible renderer for enzyme, but forking the enzyme repo so that I could fork react-shallow-renderer, or alternatively monkey patching the shallow renderer as I did in the adapter above, was painful. The unfortunate reality is that react is not built with component testability in mind.

You could probably build an adapter for enzyme using isolate-components (at least I can't think of any reasons you couldn't off the top of my head?). That would be cool. Enzyme is great. But I'm not inclined to spend my time on that given the speed of development on enzyme and react-shallow-renderer.

Enzyme moves slowly, which is understandable since it's an established project, supports old react versions, and has thousands of users whose tests could be broken if the shallow renderer suddenly started supporting effects.

This issue -- which is effectively "enzyme shallow is not useful for react 16 components" is 18 months old and there is no sign it will be addressed any time soon. An issue about transferring the shallow renderer to enzyme is 8 months old.

So, given all that, I decided to start from scratch without the baggage of supporting old react versions or depending on other packages.

@ljharb
Copy link
Member

ljharb commented Oct 26, 2020

I totally agree that React is not built with component testability in mind :-/

I'm also totally happy to remove the dependency on react-shallow-renderer/react-test-renderer, if it makes an API for shallowly testing a component more robust. If we could fix shallow to work properly on React 16.9+, say, I'd also be willing to make a major bump of enzyme that drops support for older React versions. Do you have any interest in exploring those possibilities?

@davidmfoley
Copy link

@ljharb sure; let's chat -- email me at my github name at gmail?

@dolsem
Copy link

dolsem commented Nov 20, 2020

@davidmfoley @ljharb any update on this? Should we expect a solution any time soon?

@ljharb
Copy link
Member

ljharb commented Nov 20, 2020

No update, nothing any time soon. Help is welcome.

@davidmfoley
Copy link

@dolsem we chatted about potentially making an enzyme adapter that uses isolate-components under the hood but I haven't had time since then to do more than look into the enzyme adapter interface at a high-level and see that it will be a fair amount of work. If anyone is interested in helping on that, I'd be happy to chat about it.

florianpasteur pushed a commit to hackages/wiliam-react-hackjam that referenced this issue Dec 15, 2020
limitedmage pushed a commit to sourcegraph/sourcegraph that referenced this issue Feb 1, 2021
Needed to add [jest-canvas-mock](https://www.npmjs.com/package/jest-canvas-mock) library to mock canvas on Jest, our unit test runner, as Monaco requires canvas. I tried doing a shallow test to avoid this, but unfortunately [Enzyme's shallow rendering does not call `React.useEffect`](enzymejs/enzyme#2086), which is necessary to test this functionality.
@sonicvision
Copy link

I've created a package that solves the issue - https://www.npmjs.com/package/jest-react-hooks-shallow

This works like a charm. Thanks @mikeborozdin

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
React 16
  
v16.8+: Hooks
Development

No branches or pull requests