Skip to content
This repository has been archived by the owner on Jun 17, 2021. It is now read-only.

[WIP] Component unit tests using react-testing-library #339

Closed

Conversation

idoberko2
Copy link
Collaborator

Related Issue:
#309 - react-testing-library attempt

Summary:
Examine the usage of react-testing-library to cover component testing

@idoberko2
Copy link
Collaborator Author

@AWolf81, I started with tests for Sidebar. I'm not sure it's such a good case for examination since this component doesn't have much text in it, and react-text-library is very text-oriented (its powerful tools are getByText and getByLabelText by which you can query the DOM, similar to the way the user interacts with the UI). Please have a look when you have time and I will try to add tests for another component.

@AWolf81
Copy link
Collaborator

AWolf81 commented Dec 13, 2018

@idoberko2 Thanks for adding the Sidebar test.
I've created that test for enzyme as well. I'll open a PR in a minute so it's easier to compare.

It looks OK but I'm getting an error message in every test - not sure why. It looks like following:

TypeError: Cannot read property 'now' of undefined
      at Object.<anonymous> (node_modules/wait-for-expect/lib/index.js:16:21)
      at Object.<anonymous> (node_modules/dom-testing-library/dist/wait.js:10:45)
      at Object.<anonymous> (node_modules/dom-testing-library/dist/index.js:76:13)

Seems like a configuration issue but I couldn't find the cause yet.


const { getByText } = render(<Sidebar {...props} />);

const projectButton = getByText(/Your new project was just added.*/);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getByText(/Your new project was just added.*/);

Is this supposed to be here? Is the parameter supposed to be in quotes?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks a bit like a comment but it's a regex and there is more text in that element. So it's OK like it is.
Maybe the name of the constant shoud be introductionBlurb so it's more clear that getByText is querying it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, gotcha :-) Yeah that works, or maybe just a comment above it explaining the regex (for people like me who see broken comments... 😅)

@@ -0,0 +1,61 @@
import React from 'react';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the test files have this at the top? cc @AWolf81

/* eslint-disable flowtype/require-valid-file-annotation */

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure. Do you want to avoid flow check for test files? I think // @flow is just missing here.

I think we should also use flow in our tests or maybe we should ignore them in the flow configuration if we don't need type checking.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like only platform.service.test.js and migrations.test.js have // @flow, rest are disabled. Maybe using Flow in the test files is not necessary?

@melanieseltzer
Copy link
Collaborator

I'm getting the Cannot read property 'now' of undefined errors as well. Can't seem to find anything concrete on Google. Maybe it's something to do with testEnvironment?

From skimming over the code it seems like react-testing-library is much less verbose than Enzyme which I quite like.

@idoberko2
Copy link
Collaborator Author

@AWolf81, @melanieseltzer, so what do you say? Should we drop react-testing-lib and stick with Enzyme? Do you feel like it's worth another shot?

@AWolf81
Copy link
Collaborator

AWolf81 commented Jan 3, 2019

@idoberko2 I think we should stick to Enzyme as there are more examples for tests, no issues in the setup and from the tests we've wrote there is no big difference between the two testing libs.
So I think it's better to use the more mature library.

Some other opinions to Testing-lib vs enzyme can be found in this DEV discussion.

@melanieseltzer
Copy link
Collaborator

I'm going to agree and say let's stick with Enzyme for now :-)

@AWolf81
Copy link
Collaborator

AWolf81 commented Jan 4, 2019

We can close this for now as the PR for Enzyme will be merged soon. Just waiting for the feedback from @idoberko2 to check if the Enzyme setup & the first tests are OK.

@AWolf81 AWolf81 closed this Jan 4, 2019
@AWolf81 AWolf81 mentioned this pull request Jan 6, 2019
14 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants