Skip to content

Infra/picker testing coverage #3107

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

Merged
merged 15 commits into from
Jun 27, 2024
Merged

Infra/picker testing coverage #3107

merged 15 commits into from
Jun 27, 2024

Conversation

adids1221
Copy link
Contributor

@adids1221 adids1221 commented May 29, 2024

Description

Picker testing coverage.
Testing picker modes (modal and dialog) - currently there is an issue with WheelPicker testing.
Testing picker field types by style.
Search functionality testing have the same issue as the WheelPicker.

Changelog

Picker testing coverage

Additional info

MADS-4181

@adids1221 adids1221 requested a review from M-i-k-e-l May 30, 2024 13:19
@adids1221 adids1221 marked this pull request as ready for review May 30, 2024 13:19
M-i-k-e-l
M-i-k-e-l previously approved these changes Jun 2, 2024
@M-i-k-e-l M-i-k-e-l self-requested a review June 2, 2024 07:27
@M-i-k-e-l M-i-k-e-l dismissed their stale review June 2, 2024 07:27

Missclick

@M-i-k-e-l M-i-k-e-l assigned adids1221 and unassigned M-i-k-e-l Jun 2, 2024
@adids1221 adids1221 requested a review from M-i-k-e-l June 9, 2024 13:47
@adids1221 adids1221 assigned M-i-k-e-l and unassigned adids1221 Jun 9, 2024
expect(onDismiss).toHaveBeenCalledTimes(2); // TODO: this should be 1
});

// TODO: this test is not passing yet
Copy link
Collaborator

Choose a reason for hiding this comment

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

When should this start working?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The issue here is that the onShow function is triggered only when the modal is fully open.
Iv'e tried to use act and waitFor didn't help.

I'll change the TODO comment to fix this test in the future, WDYT ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't matter IMO

// describe.skip('WheelPicker', () => {
// });

//TODO: add more tests for different props
describe('Picker field types', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

These are failing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed, bad merging from master after merging the new useFieldType hook.

Comment on lines 83 to 85
const getStyle = () => {
return StyleSheet.flatten(driver.getElement().props.style) as TextStyle;
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think you're using this anymore, do we want to keep this change?

@M-i-k-e-l M-i-k-e-l assigned adids1221 and unassigned M-i-k-e-l Jun 17, 2024
@adids1221 adids1221 requested a review from M-i-k-e-l June 17, 2024 14:09
@adids1221 adids1221 removed their assignment Jun 26, 2024
Copy link
Collaborator

@M-i-k-e-l M-i-k-e-l left a comment

Choose a reason for hiding this comment

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

@adids1221 Are you sure you've pushed your changed?

expect(onDismiss).toHaveBeenCalledTimes(2); // TODO: this should be 1
});

// TODO: this test is not passing yet
Copy link
Collaborator

Choose a reason for hiding this comment

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

Doesn't matter IMO

@M-i-k-e-l M-i-k-e-l assigned adids1221 and unassigned M-i-k-e-l Jun 27, 2024
@adids1221 adids1221 requested a review from M-i-k-e-l June 27, 2024 11:18
@adids1221 adids1221 assigned M-i-k-e-l and unassigned adids1221 Jun 27, 2024
Comment on lines +285 to +290
const label = screen.getByTestId(`${testID}.settings.type.label`);
const placeholder = screen.getByTestId(`${testID}.settings.type.placeholder`);
expect(label).toBeTruthy();
expect(placeholder).toBeTruthy();
expect(label.props.children).toEqual(undefined);
expect(placeholder.props.children).toEqual(placeholderText);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ideally the driver will return the label text and the placeholder text, please fix or add a ticket

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ticket MADS-4318

Copy link
Collaborator

@M-i-k-e-l M-i-k-e-l left a comment

Choose a reason for hiding this comment

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

Approved, please see the last note about the driver

@M-i-k-e-l M-i-k-e-l assigned adids1221 and unassigned M-i-k-e-l Jun 27, 2024
@adids1221 adids1221 merged commit c927f3a into master Jun 27, 2024
1 check passed
@adids1221 adids1221 deleted the infra/Picker_testing_coverage branch June 27, 2024 12: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.

2 participants