-
Notifications
You must be signed in to change notification settings - Fork 107
[MBL-16328][Teacher] Implement E2E test for quiz edit and preview #3405
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
base: master
Are you sure you want to change the base?
[MBL-16328][Teacher] Implement E2E test for quiz edit and preview #3405
Conversation
refs: MBL-16328 affects: Teacher release note:
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.
Review Summary
This PR adds a comprehensive E2E test for quiz editing and preview functionality. The test coverage is excellent and follows good testing practices with detailed logging. However, there are a few concerns that should be addressed:
Issues Found
-
Thread.sleep() usage (apps/teacher/src/androidTest/java/com/instructure/teacher/ui/pages/classic/EditQuizDetailsPage.kt:274-275): Using
Thread.sleep()in UI tests is an anti-pattern that can lead to flaky tests. Should use proper Espresso wait conditions or idling resources instead. -
Undocumented production code changes (apps/teacher/src/main/java/com/instructure/teacher/fragments/QuizPreviewWebviewFragment.kt:110, 120): The PR contains production code fixes that are not mentioned in the description:
- Setting
argumentson the fragment (line 110) - fixes a potential bug with fragment recreation - Adding
AUTHENTICATE = true(line 120) - changes authentication behavior for quiz previews
These changes should be explicitly documented and potentially tested separately.
- Setting
-
Test maintainability (apps/teacher/src/androidTest/java/com/instructure/teacher/ui/e2e/classic/QuizE2ETest.kt:203-220): Hardcoded date/time values scattered throughout the test. Consider extracting these into constants or well-named variables for better maintainability.
Positive Feedback
✅ Excellent test structure: The test is well-organized with clear step-by-step logging using PREPARATION_TAG, STEP_TAG, and ASSERTION_TAG
✅ Comprehensive coverage: Tests multiple aspects of quiz functionality including editing, previews, due dates, and overrides
✅ Good assertion pattern: Each assertion is clear and tests specific expected behavior
✅ Proper cleanup: The test includes removing overrides and verifying the state afterwards
✅ Accessibility support: Added content descriptions for testing (due_date_$index, due_time_$index) which aids in UI test stability
✅ New page object: The QuizPreviewPage is well-structured and follows the existing pattern
Recommendations
- Replace
Thread.sleep()calls with proper Espresso synchronization - Document the production code changes in the PR description
- Consider extracting magic numbers/dates into constants
- Add a comment explaining why
AUTHENTICATE = trueis needed for quiz previews
Overall, this is a solid addition to the test suite with comprehensive coverage. The main concerns are around test reliability (Thread.sleep) and documentation of production changes.
...teacher/src/androidTest/java/com/instructure/teacher/ui/pages/classic/EditQuizDetailsPage.kt
Show resolved
Hide resolved
apps/teacher/src/main/java/com/instructure/teacher/fragments/QuizPreviewWebviewFragment.kt
Show resolved
Hide resolved
apps/teacher/src/main/java/com/instructure/teacher/fragments/QuizPreviewWebviewFragment.kt
Outdated
Show resolved
Hide resolved
apps/teacher/src/androidTest/java/com/instructure/teacher/ui/e2e/classic/QuizE2ETest.kt
Show resolved
Hide resolved
📊 Code Coverage Report✅ Student
✅ Teacher
✅ Pandautils
📈 Overall Average
|
🧪 Unit Test Results✅ 📱 Student App
✅ 📱 Teacher App
✅ 🌅 Horizon
✅ 📦 Submodules
📊 Summary
Last updated: Wed, 03 Dec 2025 10:12:47 GMT |
tamaskozmer
left a 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.
As I mentioned remove the production code changes, it seems like it is not needed. If the tests still fails occasionally investigate the source or find a workaround to apply only in the test code if this only happens in testing.
…ent. refs: MBL-16328 affects: Teacher release note:
|
|
||
| class QuizPreviewPage : BasePage(R.id.canvasWebView) { | ||
|
|
||
| fun assertPreviewLoaded() { |
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.
We are using the KDoc format in the teacher app, please comment up these functions like the existing ones.
| class QuizPreviewPage : BasePage(R.id.canvasWebView) { | ||
|
|
||
| fun assertPreviewLoaded() { | ||
| waitForViewWithId(R.id.canvasWebView) |
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 wouldn't be sure if the canvasWebView container is loaded that implicates that the preview is loaded successfully.
I'd put some other assertions below it, like calling the assertQuizTitleDisplayed and assertQuizDescriptionDisplayed methods. (And you can use just the assertPreviewDisplayed method in the test, so the other 2 methods may could be private if you don't use them elsewhere).
| onViewWithContentDescription("due_date_$overrideIndex").scrollTo() | ||
| onViewWithContentDescription("due_date_$overrideIndex").click() | ||
| } | ||
| fun clickEditDueTime(overrideIndex: Int = 0) { |
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.
Missing empty line between the functions
| * | ||
| * @param newDescription The new description to assert. | ||
| */ | ||
| fun assertQuizDescriptionChanged(newDescription: String) { |
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 call this method 'assertQuizDescriptionDisplayed' because it is actually just checking the description, not validating change. If you use this to validate a change process in your test, then write the logs accordingly, but this method itself does not test the change, just the current value.
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.
Also, I know 'assertQuizNameChanged' is a legacy method but please refactor that one as well.
| fun clickEditDueDate(overrideIndex: Int = 0) { | ||
| addOverrideButton().scrollTo() | ||
| Thread.sleep(1000) //wait for the UI to be settled | ||
| onViewWithContentDescription("due_date_$overrideIndex").scrollTo() |
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.
You don't need to get the element twice, you can just do:
onViewWithContentDescription("due_date_$overrideIndex").scrollTo().click()
| Log.d(STEP_TAG, "Go back to Quiz Details page.") | ||
| Espresso.pressBack() | ||
|
|
||
| Log.d(STEP_TAG, "Open Due Dates section.") |
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.
You can merge the logs of these 2 steps.
| Log.d(STEP_TAG, "Click the pencil/edit icon to open the edit page.") | ||
| assignmentDueDatesPage.openEditPage() | ||
|
|
||
| Log.d(STEP_TAG, "Set due date and time for the first due date.") |
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.
It would be nice to put the values to the log as well to see what date and time will be set.
| Log.d(ASSERTION_TAG, "Assert that another new due date override has been created.") | ||
| editQuizDetailsPage.assertNewOverrideCreated() | ||
|
|
||
| Log.d(STEP_TAG, "Set due date and time for the second override.") |
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.
Same as above.
| Log.d(STEP_TAG, "Click the pencil/edit icon to open the edit page.") | ||
| assignmentDueDatesPage.openEditPage() | ||
|
|
||
| Log.d(STEP_TAG, "Remove the second due date.") |
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.
Value should be in the log to be clear what date will be removed.
| Log.d(STEP_TAG, "Save the quiz after removing the second due date.") | ||
| editQuizDetailsPage.saveQuiz() | ||
|
|
||
| Log.d(STEP_TAG, "Refresh the Due Dates page.") |
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.
you can merge this and the previous steps logs.
Summary:
Added comprehensive E2E test coverage for quiz editing and preview functionality in the Teacher app, including multi-override due date management.
Changes:
Test Coverage:
refs: MBL-16328
affects: Teacher
release note:
Checklist