react-hooks-testing-library icon indicating copy to clipboard operation
react-hooks-testing-library copied to clipboard

Unmount doesn't seem to be firing useEffect cleanup functions

Open kmarple1 opened this issue 2 years ago β€’ 15 comments

  • react-hooks-testing-library version: 8.0.0
  • react version: ^17.0.2
  • react-dom version (if applicable): ^17.0.2
  • react-test-renderer version (if applicable): n/a
  • node version: v14.17.6
  • npm (or yarn) version: 6.14.15

Relevant code or config:

The component I'm testing:

import { useState, useEffect } from "react";

const useDebounce = (value, delay) => {
    const [debouncedValue, setDebouncedValue] = useState(value);

    useEffect(() => {
        const handler = setTimeout(() => {
            setDebouncedValue(value);
        }, delay);

        return () => {
            clearTimeout(handler);
        };
    }, [value]);

    return debouncedValue;
};

export default useDebounce;

The test itself:

import { renderHook } from "@testing-library/react-hooks";
import useDebounce from "../useDebounce";

jest.useFakeTimers();
jest.spyOn(global, "clearTimeout");

describe("useDebounce", () => {
    test(`it should clear the timeout when unmounting`, () => {
        const value = "test";
        const delay = 1000;
        const { unmount } = renderHook(() => useDebounce(value, delay));
        expect(clearTimeout).not.toHaveBeenCalled();
        unmount();
        expect(clearTimeout).toHaveBeenCalled(); // fails here
    });
});

What you did:

I'm attempting to test that the useEffect cleanup function in my hook fires correctly.

What happened:

While it works properly when running normally, I can't get unmount() to fire it.

Reproduction:

The above example reproduces the problem.

Problem description:

Unmount doesn't seem to be properly cleanup up useEffects.

kmarple1 avatar May 25 '22 20:05 kmarple1

I imagine it’s the fake timers getting in the way. Try without that line and see if it works (I’m unable to test it myself at the moment)

mpeyper avatar May 27 '22 11:05 mpeyper

I have the same problem without any fake timers.

I have an RxJS observable which I unsubscribe in the effect cleanup. Works in the browser, but not when running tests.

AndyOGo avatar Sep 09 '22 18:09 AndyOGo

I'm also having this issue, I call a function on the window in my effect cleanup but I can't asset that it's being called as the clean up function never get's called when unmounting.

Is anyone looking into this? Thanks.

dahliacreative avatar Oct 19 '22 10:10 dahliacreative

Hi @dahliacreative,

Noone is currently looking into this but we have tests that ensure the effects are cleaned up, so it's unlikely to be an issue with this library or react itself (I cannot stress how much of our functionality is just calling react to do it's thing) and much more likely to be in your code. A common issue I see is that the work in the cleanup is actually async and has not run in between the unmount call and assertions happening.

If you want help diagnosing the issue, I suggest sharing some code here/a new "Question" issue/discord, or putting together a small reproduction repo if you cannot share your actual code with us.

mpeyper avatar Oct 19 '22 23:10 mpeyper

Have same issue - not firing useEffect cleanup on unmount. Here is the code snippet I am using:

Video.tsx
---------------
React.useEffect(() => {
    video.addEventListener('mouseover', onPlay);
    video.addEventListener('mouseout', onPause);
    return () => {
      video.removeEventListener('mouseover', onPlay);
      video.removeEventListener('mouseout', onPause);
    };
  }, []);
Video.spec.tsx
----------------
it('should call addEventListener on mount', () => {
    const { unmount } = render(<Video src={src} />);
    expect(addEventListener).toHaveBeenCalled();
    unmount();
    expect(removeEventListener).toHaveBeenCalled();
});

Tried wrapping unmount around act, but unmount already does that - so that's not it. Any help here is appreciated.


Edit: Okay, this worked for me:

Caching a reference to the useEffect's cleanup function

let cleanupFunc;
jest.spyOn(React, 'useEffect').mockImplementation((f) => {
    cleanupFunc = f();
});

Then finally calling this reference on unmount

it('should call addEventListener on mount', () => {
    const { unmount } = render(<Video src={src} />);
    expect(addEventListener).toHaveBeenCalled();
    unmount();
    cleanupFunc();
    expect(removeEventListener).toHaveBeenCalled();
});

harshabikkavilli avatar Nov 17 '22 04:11 harshabikkavilli

try skiping one thread cycle before expecting:

it('should call addEventListener on mount', async () => {
    const { unmount } = render(<Video src={src} />);
    expect(addEventListener).toHaveBeenCalled();
    unmount();

    // wait for a moment
    await new Promise((resolve) => setTimeout(resolve, 1));

    expect(removeEventListener).toHaveBeenCalled();
});    

vzaidman avatar Nov 30 '22 14:11 vzaidman

This did not work for me @vzaidman but what did work @kmarple1 was the following:

Component

import { useEffect } from 'react'

const Component = () => {
  useEffect(async () => {
    console.log('renders')
    return () => {
      console.log('cleanup')
    }
  }, [url])

  return <div/>
}

export default Component
  it('unmounts', async () => {
    console.log = jest.fn()
    let cleanupFuncPromise
    jest.spyOn(React, 'useEffect').mockImplementationOnce(func => cleanupFuncPromise = func())
    const subject = await render(<Component/>)
    await subject.unmount()
    const cleanUpFunction = await cleanupFuncPromise
    cleanUpFunction()
    expect(console.log).toHaveBeenCalledWith('cleanup)
  })

ignatospadov avatar Apr 18 '23 12:04 ignatospadov

Any updates on this? I don't want to mock my useEffect as my useEffect is actually making changes in my redux store, and That would turn into alot of work, Im mocking each step just for this.

Fawwad-Khan avatar May 29 '23 05:05 Fawwad-Khan

Hi @Fawwad-Khan No update from my end. If you want to share some code (or ideally a reproduction repo) I'm may be able to spot something obvious, but I don't have a lot of time for deep debugging on issues like this (I'll reiterate that it's unlikely to be this library causing the issue here).

Also, @ignatospadov, I've just noticed that your usage of useEffect here does not look valid to me. You cannot pass a async function to useEffect like that as far as I'm aware. To be valid it should looks more like:

const Component = () => {
  useEffect(() => {
    const run = async () => {
      console.log('renders')
    }

    run()

    return () => {
      console.log('cleanup')
    }
  }, [url])

  return <div/>
}

Hope that helps.

mpeyper avatar May 29 '23 05:05 mpeyper

I cant really share the code as the organization I work at doesnt let me. This is what it pretty much looks like.

Im unsetting the UIState on unmount but im still getting the "something" value after unmount in my test. It works fine in the UI/Implementation.

const MyHook = () => {

const dispatch = useDispatch()
const {UIState} = useSelector(state => ({ UIState: undefined }))

  useEffect(() => {
   dispatch(setUIState("something"))

    return () => {
     dispatch(setUIState(undefined))
    }
  }, [dispatch, setUIState])

  return {
  UIState
  }
}

Fawwad-Khan avatar May 29 '23 06:05 Fawwad-Khan

Hello, if this can be of any help, here is a reproduction code:

// src/useMount.ts

import { useEffect, useState } from 'react'

export function useMount() {
  const [mounted, setMounted] = useState(false)

  useEffect(() => {
    setMounted(true)
    return () => {
      setMounted(false)
    }
  }, [])

  return mounted
}
// tests/unit/useMount.test.ts

import { renderHook } from '@testing-library/react'
import { useMount } from '../../src/useMount'

describe('useMount', () => {
  it('should be mounted', () => {
    const { result } = renderHook(useMount)
    expect(result.current).toBe(true)
  })

  it('should not be mounted after unmount', () => {
    const { result, unmount } = renderHook(useMount)
    unmount()
    expect(result.current).toBe(false)
  })
})

Result: image

Dependencies:

    "@testing-library/dom": "^9.3.1",
    "@testing-library/react": "^14.0.0",
    "@types/jest": "^29.5.2",
    "@types/react": "^18.2.13",
    "jest": "^29.5.0",
    "jest-environment-jsdom": "^29.5.0",
    "react": "^18.2.0",
    "react-dom": "^18.2.0",
    "ts-jest": "^29.1.0",
    "typescript": "^5.1.3"

This code is hosted here: https://github.com/saramorillon/hooks/tree/reproduction-847

saramorillon avatar Jun 23 '23 07:06 saramorillon

Hello, if this can be of any help, here is a reproduction code:

// src/useMount.ts

import { useEffect, useState } from 'react'

export function useMount() {
  const [mounted, setMounted] = useState(false)

  useEffect(() => {
    setMounted(true)
    return () => {
      setMounted(false)
    }
  }, [])

  return mounted
}
// tests/unit/useMount.test.ts

import { renderHook } from '@testing-library/react'
import { useMount } from '../../src/useMount'

describe('useMount', () => {
  it('should be mounted', () => {
    const { result } = renderHook(useMount)
    expect(result.current).toBe(true)
  })

  it('should not be mounted after unmount', () => {
    const { result, unmount } = renderHook(useMount)
    unmount()
    expect(result.current).toBe(false)
  })
})

Result: image

Dependencies:

    "@testing-library/dom": "^9.3.1",
    "@testing-library/react": "^14.0.0",
    "@types/jest": "^29.5.2",
    "@types/react": "^18.2.13",
    "jest": "^29.5.0",
    "jest-environment-jsdom": "^29.5.0",
    "react": "^18.2.0",
    "react-dom": "^18.2.0",
    "ts-jest": "^29.1.0",
    "typescript": "^5.1.3"

This code is hosted here: https://github.com/saramorillon/hooks/tree/reproduction-847

Here is how this hook is implemented on the usehooks-ts library. I hope this example can provide some helpful insights! πŸ‘

bismarkhenao avatar Feb 20 '24 18:02 bismarkhenao

Anyone found the solution, here is my code where issue is happening.


  React.useEffect(() => {
    emitter.on(MY_EVENT,callback);

    return () => { 
      console.log('unsubscribing'); // <<--- Its not getting console logged.
      emitter.off(MY_EVENT, callback);
    };
  }, []);
  it.only('should add and remove event listeners', () => {
    const { unmount } = render(<Component {...defaultProps} />);
    expect(emitter.on).toHaveBeenCalledTimes(1);
    expect(observers[MY_EVENT).toBeDefined();


    unmount();
    expect(emitter.off).toHaveBeenCalledTimes(1);
    expect(observers[MY_EVENT]).toBeUndefined();
  });
});

    expect(jest.fn()).toHaveBeenCalledTimes(expected)

    Expected number of calls: 1
    Received number of calls: 0

      23 |
      24 |     unmount();
    > 25 |     expect(emitter.off).toHaveBeenCalledTimes(1);
          |                         ^
      26 |     expect(observers[MY_EVENT]).toBeUndefined();

bhushanlaware avatar Apr 02 '24 15:04 bhushanlaware

@bhushanlaware it looks like you are rendering with react-testing-library, not react-hooks-testing-library so I suggest raising an issue with them for your case.

As a general reminder to others coming into this issue, if you are importing renderHook from @testing-library/react and not @testing-library/react-hooks then you should also be raising an issue with react-testing-library instead of tacking on a +1 to this issue.

Also a general suspicion that fake timers, unhandled async or mocked useEffect implementations are the likely cause here rather than the testing libraries involved. We generally defer the actual rendering to react itself, so if effects are not being cleanup up, a lot more people would be complaining about it.

mpeyper avatar Apr 02 '24 16:04 mpeyper