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.

Sign up for GitHub

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

Jump to bottom

Fix #3446: Fixing PlatformParameterIntegrationTest for both Espresso and Robolectric [RunAllTests] #3478

Merged
merged 5 commits into from
Jul 23, 2021
Merged

Fix #3446: Fixing PlatformParameterIntegrationTest for both Espresso and Robolectric [RunAllTests] #3478

merged 5 commits into from
Jul 23, 2021

Conversation

Arjupta
Copy link
Contributor

@Arjupta Arjupta commented Jul 14, 2021

Explanation

Fixes #3446
Added Robolectric dependency for androidTestImplementation in app module. This fixed the PlatformParameterTest to run on both Espresso and Robolectric

Checklist

  • The PR title starts with "Fix #bugnum: ", followed by a short, clear summary of the changes. (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • The PR explanation includes the words "Fixes #bugnum: ..." (or "Fixes part of #bugnum" if the PR only partially fixes an issue).
  • The PR follows the style guide.
  • The PR does not contain any unnecessary auto-generated code from Android Studio.
  • The PR is made from a branch that's not called "develop".
  • The PR is made from a branch that is up-to-date with "develop".
  • The PR's branch is based on "develop" and not on any other branch.
  • The PR is assigned to an appropriate reviewer in both the Assignees and the Reviewers sections.

@Arjupta
Added Robolectric dependency for androidTestImplementation in app mod…
7d8ce37 
…ule. This fixed the PlatformParameterTest to run on both Espresso and Robolectric
jcqli
jcqli suggested changes Jul 14, 2021
View reviewed changes
Copy link
Contributor

@jcqli jcqli left a comment

Choose a reason for hiding this comment

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

This really simplified the code, looks good! Adding a blocking comment re: toast view but I'm not sure how it works with the ShadowToast.

app/src/sharedTest/java/org/oppia/android/app/testing/PlatformParameterIntegrationTest.kt Show resolved Hide resolved
@oppiabot
Copy link

oppiabot bot commented Jul 14, 2021

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

anandwana001
anandwana001 suggested changes Jul 15, 2021
View reviewed changes
Copy link
Contributor

@anandwana001 anandwana001 left a comment •
edited

Choose a reason for hiding this comment

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

Please add an espresso test result video. Also, we need to look into the other option than using Shadow.

@anandwana001 anandwana001 removed their assignment Jul 15, 2021
@vinitamurthi vinitamurthi removed their assignment Jul 15, 2021
@vinitamurthi
Copy link
Contributor

Note I am removing myself from assignees since I will not be available this weekend, if my input is needed please feel free to add me back and mention me in the comment where its required and I will take a look on Monday!

@Arjupta
Copy link
Contributor Author

Arjupta commented Jul 15, 2021

Please add an espresso test result video. Also, we need to look into the other option than using Shadow.

Espresso tests are failing with this error -
"Attempt to invoke interface method 'java.lang.Object org.robolectric.internal.bytecode.ShadowedObject.$$robo$getData()' on a null object reference"

@jcqli
Copy link
Contributor

jcqli commented Jul 15, 2021

Please add an espresso test result video. Also, we need to look into the other option than using Shadow.

Espresso tests are failing with this error -
"Attempt to invoke interface method 'java.lang.Object org.robolectric.internal.bytecode.ShadowedObject.$$robo$getData()' on a null object reference"

Is this a new failure? What changes were made?

@anandwana001
Copy link
Contributor

Shadow doesn't work with espresso, it's a robolectric library

@Arjupta
Copy link
Contributor Author

Arjupta commented Jul 15, 2021

@anandwana001 as per @BenHenning 's suggestion I have moved this PlatformParameterIntegrationTest to "test" directory of app module. Does this looks good to you?

@Arjupta
Copy link
Contributor Author

Arjupta commented Jul 15, 2021

Please add an espresso test result video. Also, we need to look into the other option than using Shadow.

Espresso tests are failing with this error -
"Attempt to invoke interface method 'java.lang.Object org.robolectric.internal.bytecode.ShadowedObject.$$robo$getData()' on a null object reference"

Is this a new failure? What changes were made?

There were no new changes, only ran these tests on Espresso. Earlier I was more focussed on resolving the failing tests on robolectric thatswhy I didn't ran them for Espresso.

@Arjupta Arjupta assigned anandwana001 and unassigned Arjupta Jul 15, 2021
jcqli
jcqli approved these changes Jul 19, 2021
View reviewed changes
Copy link
Contributor

@jcqli jcqli left a comment

Choose a reason for hiding this comment

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

LGTM

@rt4914
Copy link
Contributor

rt4914 commented Jul 19, 2021

Un-assigning myself and will defer to @anandwana001 on this PR.

@rt4914 rt4914 removed their assignment Jul 19, 2021
anandwana001
anandwana001 suggested changes Jul 22, 2021
View reviewed changes
app/src/test/java/org/oppia/android/app/testing/PlatformParameterIntegrationTest.kt Outdated
@@ -109,6 +91,7 @@ class PlatformParameterIntegrationTest {
@Before
fun setUp() {
setUpTestApplicationComponent()
ShadowToast.reset()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we resetting at the start of the test?

Reset method is called when we want to clear the recorded test, mostly after functions like .showToast() or after functions like where to count or read the toast text.

Copy link
Contributor

Choose a reason for hiding this comment

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

Screenshot 2021-07-22 at 18 55 00

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 was doing it just to clear any previous state. But yeah if we can work without it thats not an issue. Commiting this change

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know removing will work or not, I just shifted it to after. Let's see if it works, though moving it to after works, because let's say in future if we add more test with more toast text, it might fail.

@anandwana001 anandwana001 assigned Arjupta and unassigned anandwana001 Jul 22, 2021
@Arjupta Arjupta assigned anandwana001 and unassigned Arjupta Jul 22, 2021
@Arjupta
Copy link
Contributor Author

Arjupta commented Jul 22, 2021

@anandwana001 PTAL, it can be merged now.
//cc @vinitamurthi @jcqli

@vinitamurthi
Copy link
Contributor

Note the bazel tests are probably not running because no test targets were affected by this change

@vinitamurthi vinitamurthi removed their assignment Jul 23, 2021
@Arjupta Arjupta changed the title Fix #3446: Fixing PlatformParameterIntegrationTest for both Espresso and Robolectric Fix #3446: Fixing PlatformParameterIntegrationTest for both Espresso and Robolectric [RunAllTests] Jul 23, 2021
anandwana001
anandwana001 approved these changes Jul 23, 2021
View reviewed changes
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.

LGTM

@anandwana001 anandwana001 merged commit 2ed701c into develop Jul 23, 2021
@anandwana001 anandwana001 deleted the platform-parameter-integration-test-fix branch July 23, 2021 08:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@anandwana001 anandwana001 anandwana001 approved these changes

@jcqli jcqli jcqli approved these changes

@rt4914 rt4914 Awaiting requested review from rt4914

Assignees

@anandwana001 anandwana001

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Robolectric failure for Toast message (Reference -PlatfromParameterIntegrationTest)
5 participants
@Arjupta @vinitamurthi @jcqli @anandwana001 @rt4914