-
-
Notifications
You must be signed in to change notification settings - Fork 595
chore(iOS, Fabric): add tests for header interactions #2848
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
chore(iOS, Fabric): add tests for header interactions #2848
Conversation
kkafar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The initiative is great. We definitely need this.
This looks very solid, I've just left few style-related remarks.
Great job.
|
|
||
| describe('backButtonMenuEnabled: true', () => { | ||
| describe('backButtonDisplayMode: default', () => { | ||
| it('with default text', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| it('with default text', async () => { | |
| it('default back title stays visible', async () => { |
Just one note to all the tests - would be nice if the string in it better described our expectations regarding the test, so that it is more readable from terminal what the test does. I'm not claiming that my suggestion is even good, but would be nice to come up with something more readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed in: d1c0127
| await element(by.text('DisabledDefaultCustomText')).tap(); | ||
| await element(by.text('Open screen')).tap(); | ||
| await expect(element(by.text('Custom'))).toBeVisible(); | ||
| await expectBackButtonMenuToNotExistOnLabel('Custom'); // Check if backButtonMenu is disabled | ||
| await element(by.text('Pop to top')).tap(); // Go back |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this can be refactored out to a function with some parameters, right? We do not need to repeat it every single time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed in: d1c0127
| }); | ||
|
|
||
| // Custom | ||
| it('Default long back label should be truncated to generic by buckButtonDisplayMode', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These titles are very nice
kkafar
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job!
Description
This PR adds tests for #2809, #1589 and #2800.
Changes
The main assumption is to test all permutation of headerBackButtonMenuEnabled, headerBackButtonDisplayMode and content of button (default - taken from previous screen headerTitle, custom from headerBackTitle and styled when there are styles applied). I hope this will help us tracking regression as this part of the code is a bit confusing and caused us problem in the past.
Additionally there are added "custom" scenarios that test default default behavior with default text and custom text (if the back button changes it's appearance depending on available space and headerBackButtonDisplayMode specification).
The some of the last cases are set to failing, and this is known disparity reported in #2809, we plan to fix it soon after, but wonted to make sure we don't cause a regression.
Remark:
I did it in a way that after each test we're returning to the screen with all test cases for 2809, this is due to the fact that scrolling through list of all Test* takes a lot and reloading RN after each test would cause huge overhead. The downside of it is that if any of the test fails if will cascade fail all other.
Test code and steps to reproduce
You can run test with:
Checklist