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

Refactor: use testing library on unit tests #13

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions jest.config.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
module.exports = {
preset: 'react-native',
setupFilesAfterEnv: ['./jest.setup.js'],
moduleFileExtensions: ['ts', 'tsx', 'js', 'jsx', 'json', 'node'],
modulePathIgnorePatterns: ['<rootDir>/lib/'],
testMatch: ['**/?(*.)+(test).js'],
Expand Down
3 changes: 3 additions & 0 deletions jest.setup.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
import { toHaveStyle } from '@testing-library/jest-native';

expect.extend({ toHaveStyle });
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@
"@react-native-community/eslint-plugin": "^1.1.0",
"@release-it/conventional-changelog": "^1.1.4",
"@testing-library/jest-native": "^3.1.0",
"@testing-library/react-native": "^7.2.0",
"@types/highlight-words-core": "^1.2.0",
"@types/jest": "^25.2.3",
"@types/react-native": "^0.62.12",
Expand Down
44 changes: 39 additions & 5 deletions src/__tests__/__snapshots__/index.test.js.snap
Original file line number Diff line number Diff line change
@@ -1,15 +1,36 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`<HighlightText /> should render properly 1`] = `
exports[`<HighlightText /> should detect all ocurrencies of searched words in textToHighlight prop 1`] = `
<Text>
In this text we can do a
I'm
<Text>
hello
Slim
</Text>
to the entire

<Text>
world
Shady
</Text>
, yes, I'm the real
<Text>
Shady
</Text>
. All you other
<Text>
Slim
</Text>

<Text>
Shady
</Text>
s, are just imitating. So won't the real
<Text>
Slim
</Text>

<Text>
Shady
</Text>
, please stand up?
</Text>
`;

Expand Down Expand Up @@ -74,3 +95,16 @@ exports[`<HighlightText /> should render properly with custom styles 1`] = `
</Text>
</Text>
`;

exports[`<HighlightText /> should wrap each matched search word with an isolated element 1`] = `
<Text>
In this text we can do a
<Text>
hello
</Text>
to the entire
<Text>
world
</Text>
</Text>
`;
63 changes: 43 additions & 20 deletions src/__tests__/index.test.js
Original file line number Diff line number Diff line change
@@ -1,30 +1,53 @@
import React from 'react';
import { Text } from 'react-native';
import { create } from 'react-test-renderer';
import { render } from '@testing-library/react-native';

import HighlightText from '../';

describe('<HighlightText />', () => {
it('should render properly', () => {
const wrapper = create(
<HighlightText
searchWords={['hello', 'world']}
textToHighlight={'In this text we can do a hello to the entire world'}
/>
);
expect(wrapper.toJSON()).toMatchSnapshot();
it('should wrap each matched search word with an isolated element', () => {
const props = {
searchWords: ['hello', 'world'],
textToHighlight: 'In this text we can do a hello to the entire world',
};
const { queryByText, toJSON } = render(<HighlightText {...props} />);

for (const word of props.searchWords) {
expect(queryByText(word)).toBeTruthy();
Copy link
Contributor

Choose a reason for hiding this comment

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

Best to use getBy for check existence and queryBy* for check if is missing

Suggested change
expect(queryByText(word)).toBeTruthy();
expect(getByText(word)).toBeTruthy();

ref: https://kentcdodds.com/blog/common-mistakes-with-react-testing-library#using-query-variants-for-anything-except-checking-for-non-existence

Copy link
Author

@pedrorosarioo pedrorosarioo Oct 16, 2021

Choose a reason for hiding this comment

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

Hmmm, doesn't make any sense to test if the value returned by a getBy... is truthy, since it throws an error when it is not (docs). The reason for an eventual failure would never be because the value is falsy, so we never have to check if a value returned by a get is truthy, that's why we don't have any example of it on docs, neither on Kent C. Dods article. That approach only applies to `queryBy', because it returns null when there is no matches, which is a falsy value... What is wanted here is to check the organization of the elements, thats why we are using queryBy, we don't want to make any manipulation over the element, we are not checking the content, only checking if the library is organizing the components as expected (i.e wrapping the searched words into a single element on DOM).

Copy link
Contributor

@lucianomlima lucianomlima Oct 16, 2021

Choose a reason for hiding this comment

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

It's precisely because your test fails that you should use it, this way there is no chance for you to get a false positive. The getBy* queries should always return true, otherwise your test breaks.

Anyway, it's indicated by the creator of the library. Perhaps he deserves some credit for that suggestion. 😅

Copy link
Author

Choose a reason for hiding this comment

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

Maybe you are not thinking about the code. Why would I do a getByText without store or use it for nothing? Sorry it really didn't convince me to check a condition that can not be different, sounds so like an anti-pattern check if something that will alway return a truthy value is doing this 😅

This is not about the creator, I really think he did and does such a good job, maybe you're just making a mistaken interpretation for this situation.

Copy link
Author

@pedrorosarioo pedrorosarioo Oct 16, 2021

Choose a reason for hiding this comment

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

Maybe if I rephrase my question it gets more clear: If the test will always fail before reach the verification (toBeTruthy) when it is not truthy, why would I need to add this verification?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe if I rephrase my question it gets more clear: If the test will always fail before reach the verification if the value is truthy or falsy when it is not truthy, why you I need the verification?

Tests will not fail before reach the verification, will fail because it's impossible to check. The element that was expected to exists, don't exists.

You will check if the component that is expected to be rendered, was really rendered. Check if something that returns true , is really true is only the way to check. Could be checked if returns 1. Will be the same scenario, if not returns what is expected, should be treated as an error.

Imagine a screen where a person can update her data. Is expected to found a submit button, but for some reason, the button is not rendered. Is this an error?
How this scenario impacts user? Can she finishes the data update process?

For me, cases like that shoud be treated as an error and will be the same for the tests.

Copy link
Author

@pedrorosarioo pedrorosarioo Oct 16, 2021

Choose a reason for hiding this comment

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

Tests will not fail before reach the verification, will fail because it's impossible to check. The element that was expected to exists, don't exists.

Hmmm, I think you're wrong here. This way, the test WILL fail before reach the verification. The get methods already check if the value is truthy or falsy for us, so we don't need to check if the return of a get is truthy again, if it didn't throw error, so it is already truthy. Do a quick test: write a suite to test if some element exists on the DOM. Note that if you use expect(getByText('my-element-label').toBeTruthy(), there is no way for this test to fail because it was expecting a truthy value then received a falsy value. It happens because when javascript try to evaluate getByText('my-element-label') it will throw an error, and it occurs BEFORE the evaluation of the whole expect, because javascript starts evaluating the leaf nodes (in this case, the inner function calls first). Did it make sense for you?

Copy link
Contributor

Choose a reason for hiding this comment

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

I got it!

}

expect(toJSON()).toMatchSnapshot();
});

it('should detect all ocurrencies of searched words in textToHighlight prop', () => {
const props = {
searchWords: ['Slim', 'Shady'],
textToHighlight:
"I'm Slim Shady, yes, I'm the real Shady. All you other Slim Shadys, are just imitating. So won't the real Slim Shady, please stand up?",
};
const { getAllByText, toJSON } = render(<HighlightText {...props} />);

expect(getAllByText('Slim')).toHaveLength(3);
expect(getAllByText('Shady')).toHaveLength(4);

expect(toJSON()).toMatchSnapshot();
});

it('should render properly with custom styles', () => {
const wrapper = create(
<HighlightText
searchWords={['hello', 'world']}
textToHighlight={'In this text we can do a hello to the entire world'}
style={{ color: 'darkgray' }}
highlightStyle={{ color: 'blue' }}
/>
);
expect(wrapper.toJSON()).toMatchSnapshot();
const props = {
searchWords: ['hello', 'world'],
textToHighlight: 'In this text we can do a hello to the entire world',
style: { color: 'darkgray' },
highlightStyle: { color: 'blue' },
};

const { queryByText, toJSON } = render(<HighlightText {...props} />);

for (const word of props.searchWords) {
expect(queryByText(word)).toHaveStyle(props.highlightStyle);
}

expect(toJSON()).toMatchSnapshot();
});

it('should render properly with custom components', () => {
Expand All @@ -34,14 +57,14 @@ describe('<HighlightText />', () => {
const CustomHighlight = (props) => (
<Text style={{ fontWeight: 'bold' }}>{props.children}</Text>
);
const wrapper = create(
const { toJSON } = render(
<HighlightText
searchWords={['hello', 'world']}
textToHighlight={'In this text we can do a hello to the entire world'}
textComponent={CustomText}
highlightComponent={CustomHighlight}
/>
);
expect(wrapper).toMatchSnapshot();
expect(toJSON()).toMatchSnapshot();
});
});
7 changes: 7 additions & 0 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -2004,6 +2004,13 @@
ramda "^0.26.1"
redent "^2.0.0"

"@testing-library/react-native@^7.2.0":
version "7.2.0"
resolved "https://registry.yarnpkg.com/@testing-library/react-native/-/react-native-7.2.0.tgz#e5ec5b0974e4e5f525f8057563417d1e9f820d96"
integrity sha512-rDKzJjAAeGgyoJT0gFQiMsIL09chdWcwZyYx6WZHMgm2c5NDqY52hUuyTkzhqddMYWmSRklFphSg7B2HX+246Q==
dependencies:
pretty-format "^26.0.1"

"@types/babel__core@^7.1.7":
version "7.1.8"
resolved "https://registry.yarnpkg.com/@types/babel__core/-/babel__core-7.1.8.tgz#057f725aca3641f49fc11c7a87a9de5ec588a5d7"
Expand Down