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

MNT LinkField Jest tests #217

Merged

Conversation

sabina-talipova
Copy link
Contributor

@sabina-talipova sabina-talipova commented Feb 13, 2024

Description

These tests ensure that all Components are rendered correctly when passing certain parameters. Does not cover cases of creating a modal window (they will be added to Behat tests).

Parent issue

@sabina-talipova sabina-talipova force-pushed the pulls/4/linkfield-jest-test-2 branch 4 times, most recently from da22f5d to 02b2357 Compare February 13, 2024 03:17
@sabina-talipova sabina-talipova marked this pull request as ready for review February 13, 2024 03:38
@sabina-talipova sabina-talipova force-pushed the pulls/4/linkfield-jest-test-2 branch 3 times, most recently from 6afb506 to 590fe7a Compare February 13, 2024 06:12
@@ -57,6 +57,67 @@ function makeProps(obj = {}) {
};
}

describe('Render existing links', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Don't use describe(), it's unnecessary. Just use flat test(). When we did the large refactor on jest tests we went with the principles in this article - https://kentcdodds.com/blog/avoid-nesting-when-youre-testing which last linked from the react testing library docs

Remove describe() throughout this PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

})}
/>);
doResolve();
await act(async () => {
Copy link
Member

Choose a reason for hiding this comment

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

Instead of using act(), use await screen.findByText('Page title');. You may need to import screen from react-testing-library. Update all references to act() in the PR

If there' no text to find try one of the other screen find* methods - https://testing-library.com/docs/queries/about/

If nothing there will work then just use act()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

@sabina-talipova sabina-talipova force-pushed the pulls/4/linkfield-jest-test-2 branch 2 times, most recently from 729dd41 to 78aa111 Compare February 13, 2024 22:25
const { container } = render(<LinkFieldContext.Provider value={{ loading: false }}>
<LinkPickerMenu {...makeProps({ onSelect })} />
</LinkFieldContext.Provider>);
await act(async () => {
Copy link
Member

Choose a reason for hiding this comment

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

Can this be converted to use await screen.findByText() ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

const { container } = render(<LinkFieldContext.Provider value={{ loading: false }}>
<LinkPickerMenu {...makeProps({ onKeyDownEdit })} />
</LinkFieldContext.Provider>);
await act(async () => {
Copy link
Member

@emteknetnz emteknetnz Feb 13, 2024

Choose a reason for hiding this comment

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

Can this be converted to use await screen.findByText() ?

You'd put that below the await fireEvent.keyDown()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

@@ -1,6 +1,6 @@
/* global jest, test, expect, document */
import React from 'react';
import { render, act, screen } from '@testing-library/react';
import { render, screen, act } from '@testing-library/react';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import { render, screen, act } from '@testing-library/react';
import { render, screen } from '@testing-library/react';

Copy link
Contributor Author

@sabina-talipova sabina-talipova Feb 13, 2024

Choose a reason for hiding this comment

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

We use act here.
Comment says // Short wait - we can't use screen.find* because we're waiting for something to be removed, not added to the DOM. If we replace act with findText test is failed.

Updated. Replaces this part with waitFor() wrapper.

/* global jest, test */

import React from 'react';
import { render, fireEvent, act } from '@testing-library/react';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
import { render, fireEvent, act } from '@testing-library/react';
import { render, fireEvent, screen } from '@testing-library/react';

Assuming that act() can be replace with screen.findByText()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

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

Change commit and PR title prefix from ENH to MNT

@sabina-talipova sabina-talipova force-pushed the pulls/4/linkfield-jest-test-2 branch 3 times, most recently from 74912c7 to 62f2c2b Compare February 14, 2024 00:36
@sabina-talipova sabina-talipova changed the title ENH LinkField Jest tests MNT LinkField Jest tests Feb 14, 2024
Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

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

We don't need waitFor() everywhere :-) Please only add it where it's actually required.

@sabina-talipova
Copy link
Contributor Author

Updated

@emteknetnz emteknetnz merged commit 7be5928 into silverstripe:4 Feb 14, 2024
12 checks passed
@emteknetnz emteknetnz deleted the pulls/4/linkfield-jest-test-2 branch February 14, 2024 01:56
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.

2 participants