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

Added FractionInputInteractionViewTest.kt #4242

Conversation

bhaktideshmukh
Copy link
Contributor

Explanation

Fixes #4135 : Added FractionInputInteractionViewTest.kt

Essential Checklist

  • The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • Any changes to scripts/assets files have their rationale included in the PR explanation.
  • The PR follows the style guide.
  • The PR does not contain any unnecessary code changes from Android Studio (reference).
  • The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
  • The PR is assigned to the appropriate reviewers (reference).

For UI-specific PRs only

If your PR includes UI-related changes, then:

  • Add screenshots for portrait/landscape for both a tablet & phone of the before & after UI changes
  • For the screenshots above, include both English and pseudo-localized (RTL) screenshots (see RTL guide)
  • Add a video showing the full UX flow with a screen reader enabled (see accessibility guide)
  • Add a screenshot demonstrating that you ran affected Espresso tests locally & that they're passing

Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

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

@anandwana001 Can you please take a lead on this PR? I am not sure why I am getting assigned as the code-owner. (I will update this later on in a separate PR).

Thanks.

)
activityScenario.onActivity { activity ->
val pendingAnswer = activity.fractionInteractionViewModel.getPendingAnswer()
Truth.assertThat(pendingAnswer.answer).isInstanceOf(InteractionObject::class.java)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please prefer direct imports here and everywhere else.
assertThat(pendingAnswer.answer).isInstanceOf(InteractionObject::class.java)

val activityScenario = ActivityScenario.launch(
InputInteractionViewTestActivity::class.java
)
Espresso.onView(ViewMatchers.withId(R.id.test_fraction_input_interaction_view))
Copy link
Contributor

Choose a reason for hiding this comment

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

Please prefer direct imports here and everywhere else.
onView(withId(R.id.test_fraction_input_interaction_view))

@rt4914 rt4914 removed their assignment Mar 14, 2022
@oppiabot
Copy link

oppiabot bot commented Mar 14, 2022

Hi @bhaktideshmukh, it looks like some changes were requested on this pull request by @rt4914. PTAL. Thanks!

@anandwana001
Copy link
Contributor

anandwana001 commented Mar 14, 2022

@bhaktideshmukh

Today, it's tested through InputInteractionViewTestActivity, but these tests ought to be moved to a 
dedicated test suite for fraction input and other fraction-specific tests should be added.

After reading the issue description, what I understand is currently we use InputInteractionViewTestActivity activity to test different interactions. [FractionInputInteractionView], [NumericInputInteractionView],and [TextInputInteractionView].
As for this issue, we need to move the fraction part completely to the new test suite.

So, what we will require here is a new FractionInputInteractionViewTestActivity which does all the initialization work for the fraction part and all the current fraction tests which are using InputInteractionViewTestActivity must use the new fraction specific test activity.

Also, as part of this issue, we need to write new test cases which cover all the edge cases for fractions.
and other fraction-specific tests should be added.

Also, check this comment here - #4135 (comment)

@BenHenning Let me know if I misunderstand anything here.

Copy link
Contributor

@anandwana001 anandwana001 left a comment

Choose a reason for hiding this comment

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

Left nit thought and a comment above.

  1. Update InputInteractionViewTestActivity to remove all the fraction part.
  2. Add test result screenshots.

@DisableAccessibilityChecks
fun testFractionInput_withWholeNumber_hasCorrectPendingAnswer() {
val activityScenario = ActivityScenario.launch(
InputInteractionViewTestActivity::class.java
Copy link
Contributor

Choose a reason for hiding this comment

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

Replace with the new Fraction specific test activity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replace with the new Fraction specific test activity.

I tried the way you suggested, by creating FractionInputInteractionViewTestActivity to replace InputInteractionViewTestActivity but now tests are failing in FractionInputInteractionViewTest.kt file.

Screenshot from 2022-03-19 00-30-43

Specific error:--
'java.lang.RuntimeException: Unable to resolve activity for Intent { act=android.intent.action.MAIN cat=[android.intent.category.LAUNCHER] cmp=org.oppia.android/.app.testing.FractionInputInteractionViewTestActivity } -- see robolectric/robolectric#4736 for details'

Please do suggest how should I proceed?

Copy link
Contributor

@anandwana001 anandwana001 Mar 19, 2022

Choose a reason for hiding this comment

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

Could you share your FractionInputInteractionViewTestActivity?

Also, did you add it in the manifest file? like <activity android:name=".app.testing.InputInteractionViewTestActivity" />

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have pushed the recent changes.

qualifiers = "port-xxhdpi"
)
@LooperMode(LooperMode.Mode.PAUSED)

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

Copy link
Contributor

Choose a reason for hiding this comment

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

remove the empty line above.

@anandwana001 anandwana001 removed their assignment Mar 14, 2022
Copy link
Contributor

@anandwana001 anandwana001 left a comment

Choose a reason for hiding this comment

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

Update the InputInteractionViewTestActivity as well by removing the logic of FractionInteractionViewModel as this PR is all about separating out the fraction from it.

Left few more suggestions.

Also, check for failing github actions and update the PR with the latest develop branch.

import org.oppia.android.app.translation.AppLanguageResourceHandler
import org.oppia.android.domain.translation.TranslationController
import javax.inject.Inject

Copy link
Contributor

Choose a reason for hiding this comment

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

Add comment.

/**
 * This is a dummy activity to test input interaction views.
 * It contains [FractionInputInteractionView].
 */

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@Inject
lateinit var translationController: TranslationController

val fractionInteractionViewModel by lazy {
Copy link
Contributor

Choose a reason for hiding this comment

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

This class is missing onCreate function.
Take a look at the similar function in the InputInteractionViewTestActivity. Add the UI logic too.

Also, see activity_input_interaction_view_test, how we are binding the UI with the InputInteractionViewTestActivity. Now, for this PR, we need to separate out the fraction part completely in the new UI file as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

)
@LooperMode(LooperMode.Mode.PAUSED)

class FractionInputInteractionViewTest {
Copy link
Contributor

Choose a reason for hiding this comment

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

Run these tests on espresso and add a screenshot in the PR description showing that all the tests are passing.

qualifiers = "port-xxhdpi"
)
@LooperMode(LooperMode.Mode.PAUSED)

Copy link
Contributor

Choose a reason for hiding this comment

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

remove the empty line above.

@Singleton
@Component(
modules = [
StateFragmentTest.TestModule::class, RobolectricModule::class, PlatformParameterModule::class,
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this? StateFragmentTest.TestModule

Comment on lines 274 to 276
<activity
android:name=".app.testing.FractionInputInteractionViewTestActivity"
android:theme="@style/OppiaThemeWithoutActionBar" />
Copy link
Contributor

Choose a reason for hiding this comment

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

<activity android:name=".app.testing.FractionInputInteractionViewTestActivity" />

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, keep it below the line -

<activity android:name=".app.testing.InputInteractionViewTestActivity" />

Good for readability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

)
@LooperMode(LooperMode.Mode.PAUSED)

class FractionInputInteractionViewTest {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
class FractionInputInteractionViewTest {
class FractionInputInteractionViewTestActivityTest {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why do we need to change the name?

InteractionAnswerErrorOrAvailabilityCheckReceiver {

@Inject
lateinit var resourceHandler: AppLanguageResourceHandler
Copy link
Contributor

Choose a reason for hiding this comment

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

We had introduced inject function for this file in ActivityComponentImp, but we are not calling it.

The way we call the inject function is

(activityComponent as ActivityComponentImpl).inject(this)

without this, these @inject dependency injections will not work.

@bhaktideshmukh
Copy link
Contributor Author

@anandwana001
I did all the changes the way you suggested but while running the tests I am facing this problem:--
'API level 30 is not available'
how can I fix this?

Screenshot from 2022-03-22 23-07-22

@anandwana001
Copy link
Contributor

Is you PR updated with the latest develop branch? If not let's try update and re run.

@anandwana001 anandwana001 removed their assignment Mar 25, 2022
@bhaktideshmukh
Copy link
Contributor Author

Is you PR updated with the latest develop branch? If not let's try update and re run.

Its still the same

@bhaktideshmukh
Copy link
Contributor Author

@anandwana001 I guess I have messed this PR So I will release a new and clean PR for this issue with all your previously suggested changes.

@bhaktideshmukh bhaktideshmukh deleted the fraction_input_interation_view_test branch April 6, 2022 03:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add tests for FractionInputInteractionView
3 participants