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

ALS-1927 Optimize all E2E test case #102

Merged
merged 6 commits into from
Feb 10, 2025
Merged

Conversation

wadhawh
Copy link
Contributor

@wadhawh wadhawh commented Jan 28, 2025

Issue #, if available:

Description of changes:
As per the ask E2E test cases optimized and removed all unnecessary Thread.sleep calls. When there is a system-generated dialogue, we don't have access to it. In that case, Thread.sleep is needed. ex: Location permission.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

suite_1
suite_2
suite_3
suite_4
suite_5

onView(withId(R.id.et_search_country)).perform(replaceText(TEST_WORD_ARG))

Thread.sleep(DELAY_1000)
uiDevice.wait(Until.hasObject(By.text(mActivityRule.activity.getString(R.string.description_arg))), DELAY_5000)

Choose a reason for hiding this comment

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

For my own curiosity: What's the difference between

  1. uDevice.wait
  2. waitForView
  3. mActivityRule.activity.findViewById

and when do you use each? I'm wondering why our method of testing if an element is visible or not isn't standardized across tests?

Choose a reason for hiding this comment

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

I also see some usages of uDevice.wait replaced with waitForView. Is it possible to move entirely to one convention? Or is it use case dependent?

Copy link
Collaborator

Choose a reason for hiding this comment

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

uDevice.wait: This comes with UI Automator and waits for a UI element to appear within a given period.
waitForView: Ensures a view is present before interacting with it in UI tests.
mActivityRule.activity.findViewById: Directly accesses a view from the activity in tests.

I am standardizing most instances with waitForView and removing unnecessary uiDevice.wait calls. However, in some cases, findViewById is required.

For Example, findViewById is needed to check the value inside an element.

Comment on lines 59 to 61
waitForView(allOf(withId(R.id.rv_tracking), isDisplayed()))
val rvTracking =
mActivityRule.activity.findViewById<RecyclerView>(R.id.rv_tracking)

Choose a reason for hiding this comment

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

minor: A lot of such code can be simplified if the waitForView utility were to just return the view when it's available.

val rvTracking = waitForView(allOf(withId(R.id.rv_tracking), isDisplayed()))

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fixed

uiDevice.wait(
Until.hasObject(By.text(mActivityRule.activity.getString(R.string.label_start_tracking))),
DELAY_1000
DELAY_5000

Choose a reason for hiding this comment

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

This isn't really an optimization right? We're avoiding the Thread.Sleep and moving the wait. I'm not suggesting this is a problem or that it needs to change, but just something to consider if this can be reduced further.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fixed

onView(withId(R.id.et_search_country)).perform(replaceText(TEST_WORD_RUS))

Thread.sleep(DELAY_1000)
uiDevice.wait(Until.hasObject(By.text(mActivityRule.activity.getString(R.string.description_rus))), DELAY_5000)

Choose a reason for hiding this comment

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

Is 5000 the delay needed for this element to actually show up? Seems like we went from 1000 to 5000 instead.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Fixed

Comment on lines +165 to +176
waitForView(
CoreMatchers.allOf(
withId(R.id.rv_geofence),
isDisplayed(),
hasMinimumChildCount(1),
),
)
onView(withId(R.id.rv_geofence)).perform(
RecyclerViewActions.scrollTo<RecyclerView.ViewHolder>(
hasDescendant(withText(geofenceName)),
),
)

Choose a reason for hiding this comment

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

minor: Thanks for the all the indentation/styling fixes! It would be nice to see if we can add static analysis (like FindBugs) on this package to flag these as potential warnings. That way these are fixed when the code is initially committed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Noted.

Ideally we should create a ticket and when bandwidth allow we can add it.

@wadhawh wadhawh merged commit 00a6765 into main Feb 10, 2025
2 checks passed
@wadhawh wadhawh deleted the ALS-1927_optimize_E2E_test_cases branch February 10, 2025 16:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants