Fixes oppia#5815: Add tests for PRs #4259 and #4271#6306
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Neer-rn
left a comment
Neer-rn
left a comment
There was a problem hiding this comment.
Thanks @adanaamir, the overall approach is right but I have left few comments please address them and then assign back to me.
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
I think ktlint would fail here. Did you test this locally? The closing parenthesis should be indented by 8 spaces, not 6. I think this indentation issue appears in multiple plaves, so please fix all of them and push the code only once ktlint passes locally.
|
|
||
| verify(mockRouteToResumeLessonListener) | ||
| .routeToResumeLesson( | ||
| anyOrNull(), anyString(), anyString(), anyString(), anyString(), anyOrNull(), anyOrNull() |
There was a problem hiding this comment.
Since resume routing is the new coverage this issue asked for consider capturing the arguments and asserting the checkpoint. Take a look what RecentlyPlayedFragmentTest does you will get the idea what I am talking about.
|
|
||
| verify(mockRouteToExplorationListener) | ||
| .routeToExploration( | ||
| anyOrNull(), anyString(), anyString(), anyString(), anyString(), anyOrNull(), anyBoolean() |
There was a problem hiding this comment.
Since a not started chapter always starts with checkpointing enabled please use eq(true) here instead of anyBoolean().
| @Test // TODO(#3245): Error -> URLSpan should be used in place of ClickableSpan | ||
| @DisableAccessibilityChecks | ||
| fun testStoryFragment_inProgressSavedChapter_clickChapter_routesToResumeLesson() { | ||
| storyProgressTestHelper.markInProgressSavedFractionsStory0Exp0( |
There was a problem hiding this comment.
This only marks the chapter as IN_PROGRESS_SAVED, but it never saves an actual checkpoint. Checkpoint retrieval then fails, so the presenter falls back to routeToExploration and the resume mock is never called. Found out this from your failing CI. Again PTAL at how RecentlyPlayedFragmentTest handles this. I think no BUILD changes are needed, and the fake clock is already set up correctly in setUp() so this should be an easy fix.
|
Unassigning @Neer-rn since the review is done. |
|
Hi @adanaamir, it looks like some changes were requested on this pull request by @Neer-rn. PTAL. Thanks! |
|
Hi @adanaamir, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
oppiabot
Bot
added
the
stale
|
@adanaamir, friendly ping on @Neer-rn's review comments. Could you please let us know when you will be able to address them? |
oppiabot
Bot
removed
the
stale
|
hello @adhiamboperes , I'm sorry i couldnt inform earlier, I've actually gotten busy with my internship. Due to which i havent been able to focus on this. I'll try to work on this if i have time, but if you want to unassign me and assign someone else, then thats fine by me. |
|
Hi @adanaamir, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
oppiabot
Bot
added
the
stale
3 participants
Overview
Fixes #5815
This PR adds the missing instrumentation tests for the
StoryFragmentPresenterrouting scenarios that were left out of PR #4271.What this PR does:
This PR adds test coverage for the following chapter navigation scenarios in
StoryFragmentTest.kt:ResumeLessonActivity.ExplorationActivity.ExplorationActivity.To support verification of resume navigation, this PR updates the test activity to forward
RouteToResumeLessonListenercallbacks to a mocked listener, following the existing pattern already used forRouteToExplorationListener.Testing
Added the following instrumentation tests:
testStoryFragment_notStartedChapter_clickChapter_routesToExploration()testStoryFragment_completedChapter_clickChapter_routesToExplorationAsReplay()testStoryFragment_inProgressSavedChapter_clickChapter_routesToResumeLesson()