-
Notifications
You must be signed in to change notification settings - Fork 121
[UI Tests] - Add load stats UI test #8310
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
Conversation
You can test the changes from this Pull Request by:
|
| func tapTimeframeTab(timeframe: String) -> MyStoreScreen { | ||
| app.cells.staticTexts[timeframe].tap() |
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.
WDYT of using the parent cell identifier instead? Example for This Month:
This will make the test less dependent on the localization during tab selection (you will still have a human-readable interval mentioned in the assertion of the chart hint text).
| func tapTimeframeTab(timeframe: String) -> MyStoreScreen { | |
| app.cells.staticTexts[timeframe].tap() | |
| func tapTimeframeTab(timeframeID: String) -> MyStoreScreen { | |
| app.cells[timeframeID].tap() |
The respective values for IDs to use in calling functions would be:
period-data-today-tabperiod-data-thisWeek-tabperiod-data-thisMonth-tabperiod-data-thisYear-tab
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.
ah yup i agree with your point, this is updated in 27a1d7c
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.
Thanks for addressing this @jostnes!
| func verifyStatsForTimeframeLoaded(timeframe: String) -> MyStoreScreen { | ||
| let textPredicate = NSPredicate(format: "label MATCHES %@", "Store revenue chart \(timeframe)") | ||
| XCTAssertTrue(app.images.containing(textPredicate).element.exists, "\(timeframe) chart not displayed") | ||
|
|
||
| return self | ||
| } |
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.
WDYT of extending this asserting function with one more check: making sure that the tab of interest is selected? Example of This Year tab being selected:
(For this you'll also need to provide the second argument with cell ID):
| func verifyStatsForTimeframeLoaded(timeframe: String) -> MyStoreScreen { | |
| let textPredicate = NSPredicate(format: "label MATCHES %@", "Store revenue chart \(timeframe)") | |
| XCTAssertTrue(app.images.containing(textPredicate).element.exists, "\(timeframe) chart not displayed") | |
| return self | |
| } | |
| func verifyStatsForTimeframeLoaded(timeframe: String, timeframeID: String) -> MyStoreScreen { | |
| let textPredicate = NSPredicate(format: "label MATCHES %@", "Store revenue chart \(timeframe)") | |
| XCTAssertTrue(app.images.containing(textPredicate).element.exists, "\(timeframe) chart not displayed") | |
| XCTAssertTrue(app.cells[timeframeID].isSelected, "\(timeframeID) cell not selected") | |
| return self | |
| } |
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'm leaving this as is, for now. thinking of a scenario where the tab is not selected (e.g. if tap didn't work), the image wouldn't change too so i think that extra check isn't necessary for this test to work
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.
Totally 👍 to leave it with no changes, just wanted to extend my thoughts about the reason for proposal: as you say, the test will still fail if tab selection did not work, because of incorrect chart name. My proposal was more about increasing the test coverage (e.g. checking for one field in the person details page VS checking for all fields). Also, it would cover a totally made up scenario, when tab switch animation did not work, but the screen itself is updated with correct chart and data. (just my thoughts behind it, let's leave it as it is now).
| func skipTillImplemented(file: StaticString = #file, line: UInt = #line) throws { | ||
| try XCTSkipIf(true, | ||
| "Skipping until test is properly implemented", file: file, line: line) | ||
| .verifyTodayStatsLoaded() |
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.
This might look redundant (and it most probably is), but I had an idea of also adding a function that selects the Today tab, and calling it before checking the Today stats.
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'm leaving this as is too, i think not adding that extra tap ensures that upon signing in the merchant would land on the expected tab (Today) instead of other tabs
pachlava
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.
Hey @jostnes !
Thanks for bringing this test is shape! It runs well locally for me, and on Buildkite as well 👍
I left three comments that would improve the test stability/coverage, IMO. Feel free to implement them or leave and
as is 🙂


Description
This test was previously skipped as it was not implemented, this is the implementation of the test. This test tests that the user should be able to navigate through all the stats tabs and the stats will be updated to the correct timeframe.
Testing instructions
Test that
test_load_stats_screenpasses locally and in CI, also feel free to test failing scenarios (updating assertion should fail the test)RELEASE-NOTES.txtif necessary.