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

Add tests #23

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Add tests #23

wants to merge 14 commits into from

Conversation

sanderfish
Copy link
Contributor

Set up the testing environment and added first tests for the Avatar component.

I wasn't sure how to test the styled-components / styled-system styles properly. As you can see we are now checking for 3em but it would be better to do this based on a variable set in the test itself.

Should we also be testing for if the component is capable of passing the url and name props?

@yeehaa123
Copy link
Contributor

Hi Sander,

Sorry, for my late reply. Let's find a better workflow together. Maybe chat soon to discuss. The tests look good.

A couple of small remarks:

  • put package-lock.json in .gitignore. normally it should be under source control, but given the fact that this is a monorepo managed by lerna, only the toplevel one is pushed to git.

  • what are the 'size is 3em' and 'contains img' tests testing exactly? It's not determined by the props, is it? I'd probably leave it out, especially since the snapshot already covers these two props.

  • In general, only tests inputs and output (in other words: given a set of inputs (props), this should be the output).

  • Taken the previous point in consideration, I'd argue that for this component a snapshot test is probably enough.

Okay, that's my feedback for now. The tests will probably become a lot more interesting, once the components are a bit more challenging.

Let's chat soon and discuss a better workflow (and again apologies for my tardiness)

JH

@sanderfish
Copy link
Contributor Author

I've revised the previous tests and added tests for Bar and Button components.

I found that a common case people tend to test components on, is if they are capable of passing/setting props. But props are not always used 1 on 1 in the component, e.g. you might use the prop to calculate a different value and render that in the component. Also, you can't always pass all props that the component is capable of setting at once.

This last scenario happens in the Button component, where you wouldn't pass both an onClick and a href. In the test I've now used a few props to test if the component is capable of setting props:

const props = {
    variant: "primary",
    children: formatTitle("click me!"),
    type: "button",
  };

  it('passes props', () => {
    const tree = shallow(<Button {...props} />);
    
    expect(tree.props()).toMatchObject(props);
  });

I'm unsure wether with testing the capability of setting props, we're really just testing if the component can set any prop, or if we're supposed to test the capability of setting each individual prop.

@yeehaa123 yeehaa123 closed this Oct 5, 2018
@yeehaa123 yeehaa123 deleted the add-tests branch October 5, 2018 14:06
@yeehaa123 yeehaa123 reopened this Oct 5, 2018
@yeehaa123 yeehaa123 restored the add-tests branch October 5, 2018 14:07
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.

None yet

2 participants