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

UIREQ-939 JEST/RTL test cases for PickupServicePointFilter #1049

Merged
merged 8 commits into from
Jul 12, 2023

Conversation

KetineniM
Copy link
Contributor

UIREQ-939 JEST/RTL test cases for PickupServicePointFilter
URL: https://issues.folio.org/browse/UIREQ-939

@github-actions
Copy link

github-actions bot commented May 19, 2023

Jest Unit Test Statistics

    1 files  ±0    41 suites  +1   1m 12s ⏱️ -11s
376 tests +3  376 ✔️ +3  0 💤 ±0  0 ±0 
377 runs  +3  377 ✔️ +3  0 💤 ±0  0 ±0 

Results for commit da4d99d. ± Comparison against base commit 4293b90.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented May 19, 2023

BigTest Unit Test Statistics

132 tests  ±0   132 ✔️ ±0   19s ⏱️ -3s
    1 suites ±0       0 💤 ±0 
    1 files   ±0       0 ±0 

Results for commit da4d99d. ± Comparison against base commit 4293b90.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@artem-blazhko artem-blazhko left a comment

Choose a reason for hiding this comment

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

Could you add appropriate changes to changelog file?

Comment on lines 5 to 17
jest.mock('@folio/stripes/components', () => ({
Accordion: jest.fn(({ name, children, onClearFilter }) => (
<div>
<div><button type="button" onClick={() => onClearFilter()} data-testid={`clear-${name}`}>Clear</button></div>
<div data-testid={`accordion-${name}`}>{children}</div>
</div>
)),
FilterAccordionHeader: jest.fn(() => <div>Filter Accordion Header</div>),
}));

jest.mock('@folio/stripes/smart-components', () => ({
MultiSelectionFilter: jest.fn(() => <div>Multi Selection Filter</div>)
}));
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add mocks to appropriate files here if they does not exist yet - test/jest/__mock__ ?

jest.mock('@folio/stripes/components', () => ({
Accordion: jest.fn(({ name, children, onClearFilter }) => (
<div>
<div><button type="button" onClick={() => onClearFilter()} data-testid={`clear-${name}`}>Clear</button></div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
<div><button type="button" onClick={() => onClearFilter()} data-testid={`clear-${name}`}>Clear</button></div>
<div>
<button
type="button"
onClick={onClearFilter}
data-testid={`clear-${name}`}
>Clear</button>
</div>

Comment on lines 42 to 44
expect(pickupServicePointsButton).toBeInTheDocument();
userEvent.click(pickupServicePointsButton);
expect(onClear).toHaveBeenCalled();
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add two separate tests for each expect?

onChange.mockClear();
onClear.mockClear();
});
it('should render the component with default props', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

The name of the test does not fit test scenario.

@sonarcloud
Copy link

sonarcloud bot commented Jun 2, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

CHANGELOG.md Outdated
@@ -24,6 +24,8 @@
* Added requestDate token. Refs UIREQ-962.
* Update `request-storage` okapi interface to `6.0` version. Refs UIREQ-963.
* UI tests replacement with RTL/Jest for src/PatronBlockModal.js. Refs UIREQ-878.
* Create Jest/RTL test for PickupServicePointFilter.js Refs UIREQ-939.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you remove empty line?

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you remove empty line?

<div>
<button
type="button"
onClick={() => onClearFilter()}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
onClick={() => onClearFilter()}
onClick={onClearFilter}

{ id: '3', name: 'Service Point 3' },
];

describe('PickupServicePointFilter', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you also check that MultiSelectionFilter is rendered and called with correct props?

);
const pickupServicePointsButton = screen.getByTestId('clear-pickupServicePoints');
userEvent.click(pickupServicePointsButton);
expect(onClear).toHaveBeenCalled();
Copy link
Contributor

Choose a reason for hiding this comment

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

Would be great to check that it was called with correct argument.

</div>
)),
AccordionSet: jest.fn(({ children }) => (
AccordionSet: jest.fn(({ children, onToggle }) => (
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not see the usage of this component inside PickupServicePointFilter. Do we really need to mock it?

@@ -33,9 +48,23 @@ jest.mock('@folio/stripes/components', () => ({
{children}
</div>
)),
ConfirmationModal: jest.fn(() => <div>ConfirmationModal</div>),
ConfirmationModal: jest.fn(({ heading, confirmLabel, cancelLabel, onConfirm, onCancel }) => (
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not see the usage of this component inside PickupServicePointFilter. Do we really need to mock it?

Datepicker: jest.fn(() => <div>Datepicker</div>),
ErrorModal: jest.fn(() => <div>ErrorModal</div>),
ErrorModal: jest.fn(({ label, content, buttonLabel, onClose }) => (
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not see the usage of this component inside PickupServicePointFilter. Do we really need to mock it?

@@ -122,7 +151,24 @@ jest.mock('@folio/stripes/components', () => ({
{children}
</div>
)),
Select: jest.fn(() => <div>Select</div>),
Select: ({ name, label, onChange, required, children, value }) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not see the usage of this component inside PickupServicePointFilter. Do we really need to mock it?

NotesSmartAccordion: jest.fn(() => null),
SearchAndSort: jest.fn(() => null),
ViewMetaData: jest.fn(() => null),
withTags: jest.fn((WrappedComponent) => (props) => <WrappedComponent {...props} />),
ProxyManager: () => (<div data-testid="proxy-manager" />),
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not see the usage of this component inside PickupServicePointFilter. Do we really need to mock it?

@@ -4,8 +4,18 @@ jest.mock('@folio/stripes/smart-components', () => ({
ClipCopy: jest.fn(() => null),
makeQueryFunction: jest.fn((value) => value),
CheckboxFilter: jest.fn(() => null),
NoteCreatePage: (props) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not see the usage of this component inside PickupServicePointFilter. Do we really need to mock it?

Copy link
Contributor

Choose a reason for hiding this comment

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

I do not see the usage of this component inside PickupServicePointFilter. Do we really need to mock it?

Copy link
Contributor

@Dmitriy-Litvinenko Dmitriy-Litvinenko left a comment

Choose a reason for hiding this comment

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

Could you please fix feedbacks?

@KetineniM
Copy link
Contributor Author

Could you please fix feedbacks?

I have fixed the feedback issues, could you please check and let me know

@Dmitriy-Litvinenko
Copy link
Contributor

Could you please fix feedbacks?

@KetineniM
Copy link
Contributor Author

Could you please fix feedbacks?

I have made all the possible changes. Please let me know if there is anything else remaining.

Comment on lines 5 to 18
Accordion: jest.fn(({ children, label, name, onClearFilter }) => (
<div>
{label}
{children}
<div>
<button
type="button"
onClick={onClearFilter}
data-testid={`clear-${name}`}
>Clear
</button>
</div>
<div data-testid={`accordion-${name}`} />
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
Accordion: jest.fn(({ children, label, name, onClearFilter }) => (
<div>
{label}
{children}
<div>
<button
type="button"
onClick={onClearFilter}
data-testid={`clear-${name}`}
>Clear
</button>
</div>
<div data-testid={`accordion-${name}`} />
</div>
Accordion: jest.fn(({ children, label, name, onClearFilter }) => (
<div data-testid={`accordion-${name}`} >
{label}
{children}
<div>
<button
type="button"
onClick={onClearFilter}
data-testid={`clear-${name}`}
>Clear
</button>
</div>
</div>

@@ -4,8 +4,18 @@ jest.mock('@folio/stripes/smart-components', () => ({
ClipCopy: jest.fn(() => null),
makeQueryFunction: jest.fn((value) => value),
CheckboxFilter: jest.fn(() => null),
NoteCreatePage: (props) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not see the usage of this component inside PickupServicePointFilter. Do we really need to mock it?

expect(MultiSelectionFilter).toBeInTheDocument();
});
it('MultiSelectionFilter should render with activeValues', () => {
const activeValue = screen.getByText('test');
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const activeValue = screen.getByText('test');
const activeValue = screen.getByText(activeValues[0]);

@@ -27,6 +27,7 @@
* Added requestDate token. Refs UIREQ-962.
* Update `request-storage` okapi interface to `6.0` version. Refs UIREQ-963.
* UI tests replacement with RTL/Jest for src/PatronBlockModal.js. Refs UIREQ-878.
* Create Jest/RTL test for PickupServicePointFilter.js Refs UIREQ-939.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you move it to the bottom of the list?

@sonarcloud
Copy link

sonarcloud bot commented Jul 12, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@artem-blazhko artem-blazhko merged commit c57ab7e into master Jul 12, 2023
5 checks passed
@artem-blazhko artem-blazhko deleted the UIREQ-939 branch July 12, 2023 11:40
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.

5 participants