-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Collection UI Test (disabled currently due to #5793) added #6160
Collection UI Test (disabled currently due to #5793) added #6160
Conversation
Codecov Report
@@ Coverage Diff @@
## master #6160 +/- ##
=========================================
Coverage ? 14.49%
Complexity ? 323
=========================================
Files ? 272
Lines ? 11039
Branches ? 1593
=========================================
Hits ? 1600
Misses ? 9311
Partials ? 128 Continue to review full report at Codecov.
|
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.
Just a few nits, otherwise looks good
Until.findObject(By.text("testcollection_1")), | ||
TestAssetHelper.waitingTime) | ||
Espresso.onView(ViewMatchers.withText("testcollection_1")).click() | ||
Thread.sleep(5000) |
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.
Having this sleep looks brittle, but I think you may have explained why this was necessary? If you must use a sleep here, perhaps could we leave a comment why it's necessary? Also, instead of leaving time integer here, could we instead define that with the other time constants? https://github.com/mozilla-mobile/fenix/blob/master/app/src/androidTest/java/org/mozilla/fenix/helpers/TestAssetHelper.kt#L18-L19 Note: Eventually, we should probably move all of them to the more generic TestHelper since they're being used for things other than test assets.
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, this sleep was for investigation purpose, good catch.
org.mozilla.fenix.ui.robots.mDevice.wait( | ||
Until.findObject(By.text("testcollection_1")), | ||
TestAssetHelper.waitingTime) | ||
Espresso.onView(ViewMatchers.withText("testcollection_1")).click() |
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.
We don't have to use Espresso.onView
, onView
is enough.
// close currently opened tab | ||
homeScreen { | ||
verifyHomeScreen() | ||
Espresso.onView(ViewMatchers.withId(R.id.close_tab_button)).click() |
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.
same for all these....
@Test | ||
fun OpenTabFromCollectionTest() { | ||
// Open one tab from Collection in the Homescreen view | ||
@Ignore("Temp disable test - see: https://github.com/mozilla-mobile/fenix/issues/5793") |
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 guess all these disabled tests work on physical device, just not on x86 (for now) at least til #5793 is resolved, true?
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.
Yes they work on physical devices when ran locally.
} | ||
|
||
@Ignore | ||
// Open 2 webpages, and save each of them to a single collection |
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 like these explanatory / summary notes in places where it's not 100% clear what the test objective is. We should do this more often so that it's easier for someone else to maintain the test in the future.
Add Collections UI test that tests:
But due to #5793, it is currently disabled. Once Collections refactoring is done by a-c, it should be enabled.