-
Notifications
You must be signed in to change notification settings - Fork 531
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 #2263: Fixes App crashes in Lessons Tab and Espresso tests in TopicLessonFragmentTest #2264
Conversation
Workflow needs to be re-run as it works when I run it in my forked workflow |
@FareesHussain can we investigate why this crash is occurring? As I know earlier it is not crashing. |
may be bindable adapter was implemented in story summary recently |
Could you link the PR which cause the crash? We can fix but need to check why things working correctly before. |
@anandwana001 It was working correctly because robolectric tests are passing and it was tested in mobile portrait |
Looking into this, Thanks for mentioning. Ah, I really messed up the configuration here by passing the faulty data. Could you run the Bazel tests, steps are listed here - #2104 (comment) |
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, Thanks @FareesHussain
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.
Thanks a lot @FareesHussain
This will need @BenHenning approval as he is the code owner. |
@anandwana001 I tried to run the Bazel test you mentioned for the first command I get the output
Could you please check |
For the second command
The output is
|
@FareesHussain It involves cloning of oppia's bazel version. I will do it for now. |
I'm fine with approving this, but can we please update #2263 with the root cause (e.g. which PR introduced the original crash)? In general, cases like this should lead to a reversion, not a fix (see https://github.com/oppia/oppia-android/wiki/Revert-&-regression-policy). |
Also, before approving: @FareesHussain please add tests in the fragment holding these views that hit each of these scenarios (e.g. landscape, tablet portrait, tablet landscape). Please add a screenshot showing these tests fail without your fix in place (to ensure they prevent this from happening again). |
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.
Requesting changes until there's a test to guard against this regressing in the future.
I've already added screenshots in this pr description please check |
@BenHenning please check Before changesFor mobile landscape the following tests does'nt run to the completions in espresso (however they pass in robolectric) testLessonsPlayFragment_loadRatiosTopic_clickExpandListIconIndex1_configurationChange_chapterListIsVisible testLessonsPlayFragment_loadRatiosTopic_configurationChange_storyName_isCorrect Running all tests of TopicLessonsFragmentTest 12 out of 13 Tests fail After Changes |
Thanks @FareesHussain. Sorry to be a bit clearer, what we actually need here is a test that results in the same crash that you're fixing. Are those Espresso tests actually resulting in the same bindable adapter issue that #2263 is describing? It's also odd that Robolectric isn't failing--it should be. @FareesHussain can you make sure that we have a test that specifically fails on Robolectric without the suggest fix in place? @anandwana001 might this be because Robolectric doesn't do rotation correctly? If so, @FareesHussain you may be able to write an explicit test for this that starts the device in landscape mode (e.g. using |
@BenHenning There is no crash when I run this on Espresso(it crashes when the Item in the recyclerview is expanded) The test stop as below. I will try adding tests corresponding test that fail in robolectric. |
Sounds like that. Robolectric is not performing rotation correctly. |
@BenHenning please check |
@@ -308,6 +311,7 @@ class TopicLessonsFragmentTest { | |||
fun testLessonsPlayFragment_loadRatiosTopic_clickExpandListIconIndex1_configurationChange_chapterListIsVisible() { // ktlint-disable max-line-length | |||
launch<TopicActivity>(createTopicActivityIntent(internalProfileId, RATIOS_TOPIC_ID)).use { | |||
clickLessonTab() | |||
onView(isRoot()).perform(orientationLandscape()) |
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 added these again cause orientation does'nt change in Espresso
even if using @config
Thanks @FareesHussain. Can you actually change the test such that:
The third test is technically a new, duplicate test of the first one (but starting in landscape). This is preferred over changing existing tests to only be landsdcape + configuration change, and should improve our behavioral coverage a bit. It looks like two tests were changed before--in that case I'd expect those two to be reverted, and duplicated to be the third class of test above (starting in landscape but performing a configuration 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.
Thanks @FareesHussain! LGTM.
Explanation
Fixes #2263
NOTE: above mentioned tests passes in robolectric
Checklist
##ScreenShots
testLessonsPlayFragment_loadRatiosTopic_configurationChange_storyName_isCorrect before changes
Espresso test failing in tablet