-
Notifications
You must be signed in to change notification settings - Fork 510
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
Fix #3446: Fixing PlatformParameterIntegrationTest for both Espresso and Robolectric [RunAllTests] #3478
Fix #3446: Fixing PlatformParameterIntegrationTest for both Espresso and Robolectric [RunAllTests] #3478
Conversation
…ule. This fixed the PlatformParameterTest to run on both Espresso and Robolectric
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.
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
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.
Please add an espresso test result video. Also, we need to look into the other option than using Shadow.
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! |
Espresso tests are failing with this error - |
Is this a new failure? What changes were made? |
Shadow doesn't work with espresso, it's a robolectric library |
@anandwana001 as per @BenHenning 's suggestion I have moved this PlatformParameterIntegrationTest to "test" directory of app module. Does this looks good to you? |
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. |
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.
LGTM
Un-assigning myself and will defer to @anandwana001 on this PR. |
@@ -109,6 +91,7 @@ class PlatformParameterIntegrationTest { | |||
@Before | |||
fun setUp() { | |||
setUpTestApplicationComponent() | |||
ShadowToast.reset() |
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.
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.
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.
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 was doing it just to clear any previous state. But yeah if we can work without it thats not an issue. Commiting this change
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 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 PTAL, it can be merged now. |
Note the bazel tests are probably not running because no test targets were affected by this change |
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.
LGTM
Explanation
Fixes #3446
Added Robolectric dependency for androidTestImplementation in app module. This fixed the PlatformParameterTest to run on both Espresso and Robolectric
Checklist