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

restore ability to instrument tests around <Link /> onClick behavior outside routing context #3635

Closed
jebeck opened this issue Jul 14, 2016 · 3 comments

Comments

@jebeck
Copy link

jebeck commented Jul 14, 2016

Opening this issue as requested by @taion in #3572.

The issue is that the warning added to help prevent accidental rendering of <Link />s outside a router context obscures the results of tests around onClick behavior. For example, both the following tests (where <Link /> is being rendered inside the <Nav /> component) will now result in clickHandler.callCount being 0, so I can't test the difference in behavior depending on the hidden prop:

  it('should not call the `clickHandler` function on link click when hidden is true', () => {
    const clickHandler = sinon.stub();
    const wrapper = mount(<Nav hidden clickHandler={clickHandler} />);
    expect(clickHandler.callCount).to.equal(0);
    wrapper.find('Link').at(0).simulate('click');
    expect(clickHandler.callCount).to.equal(0);
  });

  it('should call the `clickHandler` function on link click when hidden is false', () => {
    const clickHandler = sinon.stub();
    const wrapper = mount(<Nav hidden={false} clickHandler={clickHandler} />);
    expect(clickHandler.callCount).to.equal(0);
    wrapper.find('Link').at(0).simulate('click');
    expect(clickHandler.callCount).to.equal(1);
  });

A summary of the solutions suggested via discussion in #3572:

  1. perhaps only do the warning if not using NODE_ENV=testing (but this has the disadvantage of not catching some bugs in tests, plus using NODE_ENV=testing is not all that common)
  2. move the warning below the call to external onClick (This gets my vote! I'm already using preventDefault, so that part's not an issue.)
  3. inject a fake router context for tests like this (IMO, this detracts from the minimal test setup approach that Enzyme allows/encourages, but I can see how my usage in this case is not all that typical, so perhaps the burden is on the tester in this situation to do a little more setup.)
@taion
Copy link
Contributor

taion commented Jul 14, 2016

  1. We should move the check to after running the user onClick handler. I was being overzealous. That code needs a bit of cleanup and I'll look at it.
  2. Having to preventDefault in spies here is sort of mediocre. Does Enzyme not give you a way to inject context when you render for tests? It's very easy to do that in teaspoon, which I'm more familiar with.

@taion
Copy link
Contributor

taion commented Jul 14, 2016

That is a plug for teaspoon, by the way.

@jebeck
Copy link
Author

jebeck commented Jul 14, 2016

I'm a bit new to Enzyme, so I'll look into the context injection, and I'll look into teaspoon too 😉

@lock lock bot locked as resolved and limited conversation to collaborators Jan 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants