Enzyme: Testing component functions for clicks

[jamby]

So I'm using Karma/Jasmine for the testing.

Basically I have a component:

export default class NavBar extends Component {
  handleClick() {
    console.log("Clicked!");
  }

  render() {
    return (
      <div>
        <a role="button" onClick={this.handleClick}>
          Click this
        </a>
      </div>
    );
  }
}

And then inside my testing file:

describe("COMPONENTS: <NavBar />", function() {
  it("simulates click event for acceptingChats false", function() {
    const rendered = shallow(<NavBar {...props} />);
    const handleClick = spyOn(rendered.instance(), 'handleClick');
    rendered.find('a').simulate('click');
    expect(handleClick).toHaveBeenCalled();
  });
});

The log that I see for when I try console.log(rendered.find('a').debug()) is:

<a role="button" onClick={[Function]}>
  Click this
</a>'

Gives me the error: Expected spy handleClick to have been called. This seems to say that it is never called. I'm unsure why.

I've looked at this issue: https://github.com/airbnb/enzyme/issues/208, and various other pages for information, but I can't find how to get this to work.

Seem to be having a lot of problems with the spyOn stuff from Jasmine. Wondering if I should transfer to sinon or something of the sort.

I'm also using ReduxMockStore, and I'm wondering if I can simulate these click events with changes to the store or should I just be not worrying about Redux changes for these enzyme tests?

[ljharb]

Spies are on object properties, not on functions - as such, once the <a> is rendered, it keeps an immutable reference to the original function. You need to spyOn(NavBar.prototype, 'handleClick') _before_ you call shallow.

[jamby]

So when I do that I get this error: Error: <spyOn> : handleClick is not declared writable or has no setter.

[ljharb]

What's the implementation of NavBar look like?

[jamby]

It's above, but this is the most complete version (with using babel, ES6, and some ES7 stuff):

import React, { Component } from 'react';
import autobind from 'autobind-decorator';

@autobind
export default class NavBar extends Component {
  handleClick() {
    console.log("Clicked!");
  }

  render() {
    return (
      <div>
        <a role="button" onClick={this.handleClick}>
          Click this
        </a>
      </div>
    );
  }
}

[ljharb]

And @autobind auto-binds every method? (which, btw, is super slow; you should only autobind methods explicitly one at a time)

[jamby]

It does, and it may be, but at the moment it is fitting for us instead of having to explicitly bind them.

[ljharb]

So, specifically the implementation of that decorator is the problem. It only autobinds the first time it's referenced on the instance, so you _can't_ spy on it.

If instead, you explicitly bind in the constructor, everything will work (with spying on the prototype).

With the current implementation of your decorator, you're SOL.

[jamby]

Oh I see... well, that may sway us into removing it...

And yeah, removing it and adding the binding to the constructor fixed the issue. Thanks!

Sidenote: Doesn't binding each function directly in the constructor get messy with how the code looks? I never liked it which is why I sided with the @autobind.

[ljharb]

It's explicit, correct, and faster; which is much less messy than "having less code".

[ljharb]

Glad you got it working!

[brunoscopelliti]

Hi @ljharb,

I've a similiar issue, and would appreciate your opinion.
That's my class definition; it uses stage-2 class property transform feature.

class MyComponent extends React.Component {
    go = () => {
        // ...
    }
    render() {
        return (
            <button onClick={ this.go }>Click here</button>
        );
    }
}

And I want to spy go.

it('should call go when it is clicked', function() {
    const component = shallow(<MyComponent />);
    const goSpy = sinon.spy(component.instance(), 'go');

    component.simulate('click');

    // myMethod is executed, but ...
    sinon.assert.calledOnce(goSpy); // throws!
});

You need to spyOn(NavBar.prototype, 'handleClick') before you call shallow.

Since I'm using class property transform, go is not defined on the prototype of MyComponent; so I can't apply your advice here.

In #586 as workround is suggested to use forceUpdate; something like:

it('should call go when it is clicked', function() {
    const component = shallow(<MyComponent />);
    const instance = component.instance();
    const goSpy = sinon.spy(instance, 'go');

    // this make the trick
    instance.forceUpdate()

    component.simulate('click');

    sinon.assert.calledOnce(goSpy); // it works now!
});

I would like to ask you, what do you think about this approach?

[ljharb]

Right - that should be a prototype method, and explicitly not a class property arrow function.

In other words, go() { … }, and in the constructor, this.go = this.go.bind(this);. Then you can spy on it just fine, without needing forceUpdate.

Class properties are for data - methods are for functions. If you're installing a function as a property that's anything more than a bound method, you're doing something subpar.

[ciekawy]

do we really need to make component more complicated/verbose just to be able to make the test working? I'd actually prefer the forceUpdate() approach. Yet as there is side effect of update being called twice I wonder if there may be a way to spy on the arrow after component instance has been called yet before the render i.e. kind of sinon.stub(SomeComponent.prototype, "componentWllUpdate") yet this seems more verbose unless done as some common util.

[ljharb]

It's not just for that; it's more performant, because the optimized instance method is shared across all instances.

Arrow function class properties are an antipattern.

[ciekawy]

Actually I can see exactly same behavior with

class SomeComponent extends Component {
    constructor(props) {
        super(props);
        this.handleClick = this.handleClick.bind(this);
    }

    handleClick() {
        console.log('foo');
    }

    render() {
        return <a onClick={this.handleClick}>foo</a>
    }
}

and

import React from 'react';
import {mount, shallow} from 'enzyme';
import sinon from 'sinon';
import {expect} from 'chai';
import SomeComonent from '../../src/some-component';


describe('some component', () => {
    it('should call handleClick', () => {
        let component = shallow(<SomeComonent/>); // tried with both shallow and mount
        const instance = component.instance();
        const handleClickSpy = sinon.spy(instance, 'handleClick');
        // instance.forceUpdate(); // this is required for test to pass
        component.find('a').simulate('click');

        expect(handleClickSpy.called).to.equal(true);
    });
});

and I can imagine the reason is also the same as for arrow function - onClick={this.handleClick} is bound before the spy updates this.handleClick.

As for performance of the arrow functions - while they are assigned to instance property I don't thing there is an issue. For sure using arrow functions as instance properties is not optimal yet it seems to me as comparable solution and it is at least the least verbose one.

[ljharb]

Yes - because you should sinon.spy(SomeComponent.prototype, 'handleClick') prior to calling shallow.

It's definitely an issue, because if you create 1000 instances, you have 1000 arrow functions, whereas in the constructor-bound example, you have 1000 tiny bind-proxy-functions to 1 shared and optimized prototype function.

[ciekawy]

I'm somehow confused now as according to this jsperf test arrow functions performs significantly better then pre-bound functions on recent versions FF/Chrome/Safari. Also according to this pretty old artice arrows perform better in terms of memory as well. Was not able to find anything more details yet would be happy to read any recent detailed analysis or benchmark...

also even react blog mention using arrow property initializers

[ljharb]

jsperfs are fun toys, but micro benchmarks aren't reliable for drawing useful conclusions.

It's possible that browsers have done well optimizing the use case, but that old article predates any shipping spec-compliant arrow functions, so any performance claims it makes are invalid.

At any rate, I'm saying that constructor-bound functions are both more testable and more conceptually performant, so that's what you should use if you're having trouble with per-instance functions.

[ciekawy]

I'd really love to learn more about this, could you point me to some resources? I still guess it would be great to have it just measured in some way.

[streamich]

@jamby no need to bind anything:

class MyComponent extends Component {
    handleClick = () => {
        // ...
    };
}

[ljharb]

@streamich please read the rest of the thread: https://github.com/airbnb/enzyme/issues/697#issuecomment-265674332

[streamich]

@ljharb Why should I create an extra function at runtime just to be able to spy on it in my tests?

...you're doing something subpar.

What? Subpar is to suggest to create extra unnecessary functions.

[ljharb]

@streamich I don't think you're understanding. Using functions in class properties is incorrect, first and foremost - that it's also slower (because that is what creates unnecessary functions) and harder to test is secondary.

[streamich]

@ljharb I don't think you're understanding. You are suggesting to create two functions to handle an event. Let's say I want to handle a click event. What you are saying is I should first create a function on my component's prototype:

class MyComponent extends Component {
    onClick() {
        // ....
    }
}

And then you say I should create another function, by binding my function I just created on the prototype:

this.onClick = this.onClick.bind(this);

Effectively setting a class property :smile: as a function, which you then go and say is the incorrect way to do.

Using functions in class properties is incorrect

Why? When you bind that function, you are setting a class property as a function, right?

My approach is much more succinct and creates only one function:

class MyComponent extends Component {
    onClick = () => {
        // ....
    }
}

(because that is what creates unnecessary functions)

I guess we are now in agreement on who creates unnecessary functions?

...and harder to test is secondary.

It is not harder to test, you test it in the exact same way, because both approaches are FUNCTIONS IN CLASS PROPERTIES.

[aweary]

@streamich I think you're missing the nuance of the issue, see https://github.com/airbnb/enzyme/issues/697#issuecomment-286643955

It's definitely an issue, because if you create 1000 instances, you have 1000 arrow functions, whereas in the constructor-bound example, you have 1000 tiny bind-proxy-functions to 1 shared and optimized prototype function.

@ljharb is stating that when you bind a method you're creating a more "lightweight" proxy to the original method, while defining the function as a class property will result in a full function instance for each component instance.

Effectively setting a class property 😄 as a function, which you then go and say is the incorrect way to do.

He explicitly said (emphasis mine):

If you're installing a function as a property that's anything more than a bound method, you're doing something subpar.

[streamich]

@aweary Re: 1000 arrow functions: some components are used only once in an app, but this implies that a component is used 1000 times.

OK, let's say component is used 1000 times, this is an interesting topic, which is faster? An arrow function or Function.prototype.bind?

... when you bind a method you're creating a more "lightweight" proxy ...

Is there something I can read about this? Googling I find this:

However yes, when it matters - .bind is slower

And then Google's style guide:

Prefer using arrow functions over f.bind(this).

This jsperf shows that arrow functions and .bind are approximately equally fast (on my computer arrow function is actually 10% faster).

I mean, honestly, I don't know which one is faster, if you show me a link to a benchmark that shows that React bound functions are just "lightweight proxies" and are like 2x faster than arrow functions, I would happily use the bind method instead.

He explicitly said (emphasis mine):

If you're installing a function as a property that's anything more than a bound method, you're doing something subpar.

So how is it subpar? Especially, given that arrow function is basically a bound method in this case.

[aweary]

I'll defer to @ljharb for resources on the topic. Though I consider him to be a direct resource in cases like this since he is an active member of TC39. I imagine you can find some information in the ECMAScript spec about how bound functions are defined.

This jsperf shows that arrows functions and .bind are approximately equally fast (on my computer arrow function is actually 10% faster).

Putting aside the fact that micro-benchmarks are rarely representative of real-world performance, this benchmark isn't even testing what we're talking about. We're discussing defining methods on a class that may have a high number of instances at once (a ListItem component, for example).

Here's a benchmark that actually tests this case:
https://esbench.com/bench/59b9b35299634800a03490f7

It defines two classes, one with a bound method and the other with a class property, and then creates and tracks 1000 instances and calls the method. Testing in Chrome, I'm seeing that the bound method is >2.5 times faster.

[streamich]

@aweary In this micro-benchmark I see the bound method to be around 2x faster. However, I think it is such because (1) it is specifically constructed to favor bind; (2) I don't think it tests what we are talking about well.

(1): if you increase the number of iterations from 1K to 1M, for example, I see that arrow function is actually slightly faster.

(2): a more like a real-world scenario would be to execute results.push(instance.method(i)) many times, say 100 or 1000, in that case, I see arrow function being faster, sometimes 25%.

I guess that just supports mine and @ciekawy point that why should DX suffer just because of unsupported claims that the bind method is so much more performant, especially considering that most React components are not even used in large numbers, ListItem is probably the one exception.

Nice arrow functions, BTW:

https://github.com/aweary/react-perimeter/blob/master/src/index.js#L117

https://github.com/aweary/react-perimeter/blob/master/src/index.js#L132

[ciekawy]

The performance seems to depend significantly on the way we're benchmarking. Also there is significant difference between using just an arrow function and the requirement to bind it in a constructor especially when there is no other need for a constructor at all. I.e.

// ...
constructor(props) {
        super(props);
        this.handleClick = this.handleClick.bind(this);
    }
// ...

seems to be unnecessary syntax bloat.

Why not expect from the JS engine and/or transpiler to provide desired performance - like i.e. recent node version improved enough on delete foo.bar which allow to avoid unnecessary gimmick (it's just an example, I know here is not a right place to mention mutating an object at all ;)

[aweary]

I see that bind is faster even with 1M iterations, though that's even less representative of real-world scenarios. The benchmark is not crafted to favor bind, it's the simplest case for both scenarios. It's also testing _exactly_ what we're talking about, which is binding methods to instances in the constructor vs defining them as class properties.

Ultimately, it's up to you which approach you want to use. You should make decisions based on the actual performance of your application (which you should be measuring) not on the results of non-representative micro-benchmarks.

There may also be a difference between what's _practically performant_ and _conceptually performant_, depending on the current JS engine optimizations. What's fast now might not be fast in a few months.

Ultimately, in the context of Enzyme, there are issues with spying on functions defined as class properties. If that's an acceptable trade off for a more succinct syntax, that's your call.

[streamich]

I see that bind is faster even with 1M iterations

Are you sure? I see arrow function being 5% faster with 1M iterations, also on Chrome.

The benchmark is not crafted to favor bind...

By your logic the more iterations we add the more apparent should be performance of "one optimized original method". How come at 1K iterations bind is 2x faster but after increasing the number of iterations the performances level out?

...though that's even less representative of real-world scenarios.

The point (2) in my post dealt with being representative of real-world scenarios.

[ljharb]

Just because something (functions as class properties) is attractive, doesn't mean it's the right path, or that it has a good DX.

Using functions in class properties "because of the dev xp" is optimizing for short-term DX at the expense of long-term DX.

[ciekawy]

@ljharb, I'd really love to learn more about the right DX related to this case as I can see a lot of contradicting comments and articles without any clear directions and I've already asked this question and haven't seen here any deep arguments agains the arrow function solution.

One thing I'm personally pretty sure is that it's good to avoid excessive syntax and I'm not convinced so far there is anything fundamentally wrong with using arrow functions for this particular case which means for me it's worth to join support to make developers life easier not harder and not to prefer just one approach over another (like i.e. React vs AngularJS) but just to aim less verbose code which in general should be doable one way or another.

Using arrow functions is kind of ready solution with some drawback which should be possible to be eliminated - why not focus on this part instead of trying to cut off the discussion claiming this is just a bad design (while it does not have to be necessarily true).

[ljharb]

In no particular order, using arrow functions in class properties:

1. makes things much harder to stub out, since you can't stub the prototype prior to instantiating the element, before things have grabbed an immutable reference to the function
2. is slower, since the engine can't optimize the N function bodies (as opposed to the prototype approach where it can optimize the 1 shared body)
3. is less correct, since it's abusing class properties (meant for instance data) for the sake of binding what should conceptually be an instance method
4. is less clean than a closed-over function in the module that takes the component as an argument anyways, since it (just like a prototype method) expands the public API, since everything reachable is public, full stop
5. involves a stage 3 feature, which thus isn't yet part of the language, whereas instance methods and constructor-bindings are part of ES6 for years now

Regardless, you obviously can continue to use them - but then you're just going to be SOL for testing the functions directly, or stubbing them out. Not sure what else to tell you.

[hally9k]

I agree with your points @ljharb. Because the facts you state are correct. But I would challenge your approach to the issue at hand - Whether you like it or not a whole bunch of people seem to like their class property/arrow function combo quite a lot. The trade off between more succinct code and (potentially premature) optimisation should be in the hands of the engineer. My advise to you is to consider supporting engineers in either of the two paths that they decide to settle for. Rather than pushing the path that you've opted for on everyone else. Just my two cents.

[ljharb]

@hally9k enzyme has zero ability to obstruct users from doing that; however, there have been a large quantity of issues filed on this repo where the solution is "if you want to test it properly, you can't use arrow functions in class properties". In other words, engineers already do, and will continue to, have the decision in their own hands - but this being a testing tool, if one is interested in proper testing, arrow functions in class properties simply aren't a viable option.

[hally9k]

I suppose the difficulty here is that you know exactly how this thing ticks so it seems like a simple decision when you have the visibility. Is there a piece in the documentation that outlines the order of operations and differences between testing methods on the prototype and on the instance? This does seem to be something that trips a few people up. If not I'll see if I can get to the bottom of it and contribute. @ljharb Thanks for all the awesome work by the way, it is appreciated.

[ljharb]

There's nothing that can be done; the typical usage in user code is that they pass it into enzyme, the component installs the arrow function as a class property on the instance during the constructor, then the component is rendered, and the arrow function is passed (and thus held, by reference/identity) as a prop. Then, whether the user has spied on the method before construction, or after rendering, the spy has no effect. The proper method is to spy on the prototype method prior to creating the wrapper, bypassing the problem.

[garrettmac]

I don't think you wired your spy up correctly try moving your spy above your shallow function like this:

describe("COMPONENTS: <NavBar />", function() {
  it("simulates click event for acceptingChats false", function() {
    let handleClick = jest.spyOn(FavoriteProduct.prototype, 'handleClick');
    const rendered = shallow(<FavoriteProduct {...props} />);
    rendered.find('a').simulate('click');
    expect(handleClick).toHaveBeenCalled();
  });
});

[pgilad]

@ljharb Besides possibly sub-classing the component with arrow bound method, is there another way to stub/spy on such methods before component mount/shallow ?

[ljharb]

@pgilad the only way is to fix the implementation to avoid arrow functions in class fields.

[dmitriyK1]

I use such approach for arrow functions (instead of jest.spyOn and when I need to invoke functionality of mocked function):

class MyComponent extends Component {
    someMethod = () => {};

    otherMethod() {}

    render() {
        return <button onClick={this.someMethod}>button</button>
    }
}

const wrapper = shallow(<MyComponent />);
const instance = wrapper.instance();

instance.someMethod = jest.fn(instance.someMethod); // replace instance method with a mock function and provide original implementation for mock implementation
wrapper.find('button').simulate('click'); // this event fails to be registered via jest.spyOn() when I simulate a click on an arrow function

expect(instance.someMethod).toHaveBeenCalled();
expect(instance.otherMethod).toHaveBeenCalled(); // some side-effects from triggering mocked method call

Using this approach I can simulate clicks and they are tracked via mock(doesn't work with jest.spyOn + simulate) and original functionality of this method works as well

[deep-kiran]

Spies are on object properties, not on functions - as such, once the <a> is rendered, it keeps an immutable reference to the original function. You need to spyOn(NavBar.prototype, 'handleClick') _before_ you call shallow.

I wish this was written somewhere in docs!

[ljharb]

@deep-kiran a PR adding it would be welcome