Fixes #2785 & Fixes part of #2743: A11y activity label added #2783

rt4914 · 2021-02-25T16:07:59Z

While adding the new test cases Ben mentioned that we are using activityTestRule as well as ActivityScenario for test cases. So it would be nice if somehow we can use only of such thing and keep consistency. I tried using the ActivityScenarioRule and it works correctly in AdministratorControlsActivity and AppVersionActivity

Fixes part of #2743

This PR adds label for AdministratorControlsActivity and AppVersionActivity.

Along with that there are some minor changes in this PR to improve the code.

Output

a11y-label-1.mp4

Note for reviewers:

In general while working on #2743 I will need to add test cases across almost all activities in app module and therefore I am thinking of optimising and make more consistent test cases. Just majorly this and upcoming PRs will be focused on three things:

Add label to activity and test case.
Use ActivityScenarioRule in place of activityTestRule and ActivityScenario.
General test case optimisation and consistency.

@BenHenning and @anandwana001 WDYT about the use of ActivityScenarioRule usage?

...edTest/java/org/oppia/android/app/administratorcontrols/AdministratorControlsActivityTest.kt

app/src/sharedTest/java/org/oppia/android/app/administratorcontrols/AppVersionActivityTest.kt

anandwana001 · 2021-02-26T02:44:33Z

app/src/sharedTest/java/org/oppia/android/app/administratorcontrols/AppVersionActivityTest.kt

@@ -85,6 +82,10 @@ import javax.inject.Singleton
 )
 class AppVersionActivityTest {

+  @get:Rule
+  val activityScenarioRule: ActivityScenarioRule<AppVersionActivity> =
+    ActivityScenarioRule(createAppVersionActivityIntent())


launch function internally using ActivityScenario and calling scenario.launchInternal();.

Currently, we use ActivityScenario and this implementation is of ActivityScenarioRule, need to check the difference as I guess there is a tiny difference between them that,
ActivityScenario doesn't clean up the state after test ends but ActivityScenarioRule does it.

Also, in the ActivityScenarioRule, we cannot do any kind of setup before launching the activity, that's something we can do with ActivityScenario.

So if wee move with ActivityScenario and don't use .use{}, then we need something like

@After fun cleanup() { scenario.close() }

Need some expert points here @BenHenning

We talked about this during the team meeting today @anandwana001. I suggest looking over the team notes; there were some bits in there that were recorded from the discussion I think.

@rt4914 I'm curious about one thing: does scenario rule let us handle configuration changes correctly? I was under the impression that the scenario gets recreated when the activity does (since scenarios treat DESTROYED as a terminal state). Can you confirm?

I have made two new commits.
Commit 9d77057 shows that config change can easily be done with ActivityScenarioRule.

Commit d9fd298 shows that the code which we need before starting the activity also works correctly with ActivityScenarioRule.

Overall there is an advantage of using ActivityScenarioRule:
(a.) It can replace both ActivityTestRule and ActivityScenario and can provide consistency to all tests.
(b.) ActivityScenario does't clean up device state automatically and may leave the activity keep running after the test finishes. Call close() in your test to clean up the state or use try-with-resources statement. This is optional but highly recommended to improve the stability of your tests. Also, consider using ActivityScenarioRule. Reference

Limitation with ActivityScenarioRule:
It does not allow us to change intent inside tests which we normally does for various test cases.

Solution is to use LazeActivityScenarioRule which can allow us to change the intent for each test case. Reference

Open Questions
If we use LazeActivityScenarioRule can this create some other type of issue?
WDYT we should do finally?

Also, I highly recommend reading this blog as it summaries the difference between ActivityTestRule, ActivityScenario and ActivityScenarioRule nicely.

Please keep in mind we can't just copy LazyActivityScenarioRule--we would need to create our own rule that effectively does the same thing as ActivityScenarioRule but lets us customize the intent. I think the need here is pretty clear, but we need to define specifically what the desired API is. I suspect defaulting the intent is actually desirable for most situations, and having per-test overriding seems reasonable. However, it's not straightforward to implement that without accidentally opening multiple activities. I think we would need to start a draft PR introducing such a utility & iterate on the design a bit there to figure out something that would work for all cases. I do not recommend referencing the one from the blog--its existence introduces the idea that we can try to create on our own, but we need to be more careful about overly depending on external code that's not pre-packaged into a library.

Also, I'm not sure my original question regarding configuration changes was answered. See #2572 for specifics here--the perform orientation change doesn't actually force configuration changes in all environments (e.g. Robolectric). To answer this question, I think we need to check a test that passes in #2572 for both Espresso & Robolectric with the new utility, but also using ActivityScenarioRule.

@BenHenning Considering that this requires more research work now, can I pass in to @anandwana001 and focus back on A11y PRs. WDYT?

Yup, sgtm @rt4914.

BenHenning

Thanks @rt4914. Left some thoughts.

BenHenning · 2021-02-26T03:55:01Z

app/src/sharedTest/java/org/oppia/android/app/administratorcontrols/AppVersionActivityTest.kt

@@ -85,6 +82,10 @@ import javax.inject.Singleton
 )
 class AppVersionActivityTest {

+  @get:Rule
+  val activityScenarioRule: ActivityScenarioRule<AppVersionActivity> =
+    ActivityScenarioRule(createAppVersionActivityIntent())


We talked about this during the team meeting today @anandwana001. I suggest looking over the team notes; there were some bits in there that were recorded from the discussion I think.

@rt4914 I'm curious about one thing: does scenario rule let us handle configuration changes correctly? I was under the impression that the scenario gets recreated when the activity does (since scenarios treat DESTROYED as a terminal state). Can you confirm?

BenHenning · 2021-02-26T03:55:26Z

app/src/main/AndroidManifest.xml

@@ -84,7 +86,7 @@
      android:windowSoftInputMode="adjustResize" />
    <activity
      android:name=".app.profile.ProfileChooserActivity"
-      android:label="@string/profile_chooser_activity_label"
+      android:label="@string/profile_chooser_activity_title"


Similar to earlier PR: here & elsewhere, should these be synced to each activity's title?

rt4914 · 2021-02-26T12:19:57Z

@anandwana001 and @BenHenning PTAL to my reply above.

anandwana001

@rt4914 putting espresso test result for reference.
Nit comment left

anandwana001 · 2021-03-01T04:13:47Z

app/src/sharedTest/java/org/oppia/android/app/home/RecentlyPlayedFragmentTest.kt

@@ -185,23 +191,17 @@ class RecentlyPlayedFragmentTest {
      profileId,
      timestampOlderThanOneWeek = true
    )
-    ActivityScenario.launch<RecentlyPlayedActivity>(


aren't we removing other such launches in this class?
looks like the activity launches two times.

Wouldn't that be wrong? I'd expect Espresso/Robolectric tests to only ever verify a single activity.

We should update all tests. But this was just for example for one test.

BenHenning

Thanks @rt4914. Left some thoughts.

BenHenning · 2021-03-02T01:16:20Z

app/src/sharedTest/java/org/oppia/android/app/home/RecentlyPlayedFragmentTest.kt

@@ -185,23 +191,17 @@ class RecentlyPlayedFragmentTest {
      profileId,
      timestampOlderThanOneWeek = true
    )
-    ActivityScenario.launch<RecentlyPlayedActivity>(


Wouldn't that be wrong? I'd expect Espresso/Robolectric tests to only ever verify a single activity.

BenHenning · 2021-03-02T01:24:18Z

app/src/sharedTest/java/org/oppia/android/app/administratorcontrols/AppVersionActivityTest.kt

@@ -85,6 +82,10 @@ import javax.inject.Singleton
 )
 class AppVersionActivityTest {

+  @get:Rule
+  val activityScenarioRule: ActivityScenarioRule<AppVersionActivity> =
+    ActivityScenarioRule(createAppVersionActivityIntent())


Please keep in mind we can't just copy LazyActivityScenarioRule--we would need to create our own rule that effectively does the same thing as ActivityScenarioRule but lets us customize the intent. I think the need here is pretty clear, but we need to define specifically what the desired API is. I suspect defaulting the intent is actually desirable for most situations, and having per-test overriding seems reasonable. However, it's not straightforward to implement that without accidentally opening multiple activities. I think we would need to start a draft PR introducing such a utility & iterate on the design a bit there to figure out something that would work for all cases. I do not recommend referencing the one from the blog--its existence introduces the idea that we can try to create on our own, but we need to be more careful about overly depending on external code that's not pre-packaged into a library.

Also, I'm not sure my original question regarding configuration changes was answered. See #2572 for specifics here--the perform orientation change doesn't actually force configuration changes in all environments (e.g. Robolectric). To answer this question, I think we need to check a test that passes in #2572 for both Espresso & Robolectric with the new utility, but also using ActivityScenarioRule.

rt4914 · 2021-04-08T22:42:32Z

Closing this as it has been filed as good-first-issue #3062

A11y activity label added

d9ceb1e

rt4914 requested a review from BenHenning as a code owner February 25, 2021 16:07

Nit fix

7d56f41

rt4914 commented Feb 25, 2021

View reviewed changes

...edTest/java/org/oppia/android/app/administratorcontrols/AdministratorControlsActivityTest.kt Outdated Show resolved Hide resolved

rt4914 commented Feb 25, 2021

View reviewed changes

app/src/sharedTest/java/org/oppia/android/app/administratorcontrols/AppVersionActivityTest.kt Show resolved Hide resolved

Rajat Talesra added 4 commits February 25, 2021 22:37

Nit renaming changes

91e8f4e

Used ActivityScenerioRule

4c253d0

Nit fix

020760e

Optimised test cases

8bdd90d

rt4914 changed the title ~~Fixes part of #2743: A11y activity label added~~ Fixes #2785 & Fixes part of #2743: A11y activity label added Feb 25, 2021

rt4914 assigned BenHenning and anandwana001 Feb 25, 2021

rt4914 requested a review from anandwana001 February 25, 2021 18:41

anandwana001 suggested changes Feb 26, 2021

View reviewed changes

BenHenning reviewed Feb 26, 2021

View reviewed changes

BenHenning assigned rt4914 and unassigned BenHenning Feb 26, 2021

Rajat Talesra added 2 commits February 26, 2021 17:22

Proof for config change test case

9d77057

Proof that pre-setup works in tests

d9fd298

rt4914 assigned BenHenning and unassigned rt4914 Feb 26, 2021

anandwana001 suggested changes Mar 1, 2021

View reviewed changes

anandwana001 assigned rt4914 and unassigned anandwana001 Mar 1, 2021

BenHenning reviewed Mar 2, 2021

View reviewed changes

BenHenning removed their assignment Mar 2, 2021

rt4914 closed this Apr 8, 2021

rt4914 deleted the a11y-label-part-1 branch April 8, 2021 22:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #2785 & Fixes part of #2743: A11y activity label added #2783

Fixes #2785 & Fixes part of #2743: A11y activity label added #2783

rt4914 commented Feb 25, 2021 •

edited

Loading

anandwana001 Feb 26, 2021

BenHenning Feb 26, 2021

rt4914 Feb 26, 2021

BenHenning Mar 2, 2021

rt4914 Mar 2, 2021

BenHenning Mar 2, 2021

rt4914 Mar 2, 2021

BenHenning left a comment

BenHenning Feb 26, 2021

BenHenning Feb 26, 2021

rt4914 commented Feb 26, 2021

anandwana001 left a comment

anandwana001 Mar 1, 2021

BenHenning Mar 2, 2021

rt4914 Mar 2, 2021

BenHenning left a comment

BenHenning Mar 2, 2021

BenHenning Mar 2, 2021

rt4914 commented Apr 8, 2021

Fixes #2785 & Fixes part of #2743: A11y activity label added #2783

Fixes #2785 & Fixes part of #2743: A11y activity label added #2783

Conversation

rt4914 commented Feb 25, 2021 • edited Loading

Output

Note for reviewers:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenHenning left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rt4914 commented Feb 26, 2021

anandwana001 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenHenning left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rt4914 commented Apr 8, 2021

rt4914 commented Feb 25, 2021 •

edited

Loading