[MBL-19655][Student] - Use DataSeeding in existing Discussion Checkpoint related E2E tests #3475
Conversation
…rnt in course and discussions. Handle discussions with due dates and no due dates. Add date converting utility function. refs: MBL-19655 affects: Student release note: -
…ckpoint-tests-to-use-dataseeding
…ckpoint-tests-to-use-dataseeding
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
This PR improves E2E test coverage for discussions with checkpoints by removing hardcoded course IDs and using properly seeded data. The changes enable testing of both discussions with and without due dates.
Positive Changes
✅ Replaced hardcoded course IDs with dynamically seeded data - much better for test isolation
✅ Added comprehensive testing for checkpoints with different due date scenarios
✅ Improved test data setup by enrolling both students and teachers properly
✅ Added date conversion utility to handle ISO8601 format consistently
✅ Simplified DateTime scalar handling by treating it as String instead of Date object
Issues Found
-
Test flakiness concern (apps/student/src/androidTest/java/com/instructure/student/ui/e2e/classic/DiscussionsE2ETest.kt:224) -
sleep(2000)introduces non-deterministic behavior. Use proper wait mechanisms instead. -
Missing error handling (automation/espresso/src/main/kotlin/com/instructure/espresso/TestingUtils.kt:84) -
SimpleDateFormat.parse()can throwParseExceptionwhich isn't handled. Also consider caching the formatter for performance. -
Code complexity (automation/espresso/src/main/kotlin/com/instructure/canvas/espresso/common/pages/compose/AssignmentListPage.kt:166) - The
assertHasAssignmentCommon()function has deeply nested conditionals that would benefit from refactoring into helper functions. -
Magic string duplication (automation/espresso/src/main/kotlin/com/instructure/canvas/espresso/common/pages/compose/AssignmentListPage.kt:180) - The hardcoded time string " 11:59 PM" is repeated multiple times. Extract as a constant.
Test Coverage
The test coverage looks comprehensive with scenarios for:
- Discussions with checkpoints without due dates
- Discussions with checkpoints with due dates
- Assignment list display validation
- Syllabus summary event validation
- Assignment details page validation
Overall Assessment
Good refactoring work to improve test maintainability. The main concerns are around test stability (the sleep call) and code maintainability (error handling, complexity, duplication). These are relatively minor issues that should be straightforward to address.
| assignmentListPage.clickAssignment(discussionWithCheckpointsTitle) | ||
| Log.d(STEP_TAG, "Click on the expand icon for the '$discussionWithCheckpointsWithoutDueDatesTitle' discussion (to see the checkpoints' details).") | ||
| assignmentListPage.clickDiscussionCheckpointExpandCollapseIcon(discussionWithCheckpointsWithoutDueDatesTitle) | ||
| sleep(2000) // Allow some time for the collapse action to propagate |
There was a problem hiding this comment.
This sleep() call is a code smell in tests. It introduces flakiness and makes tests slower. Consider using Espresso's idling resources or composeTestRule.waitForIdle() instead of hard-coded sleep delays.
| val iso8601Format = SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss'Z'", Locale.getDefault()).apply { | ||
| timeZone = TimeZone.getTimeZone("UTC") | ||
| } | ||
| val date = iso8601Format.parse(iso8601Date) ?: throw IllegalArgumentException("Invalid date format: $iso8601Date") |
There was a problem hiding this comment.
The parse() method can throw ParseException which isn't being handled. Consider wrapping in a try-catch block or documenting this behavior. Also, creating new SimpleDateFormat instances repeatedly can be inefficient - consider caching the formatter as a constant.
| } | ||
|
|
||
| private fun assertHasAssignmentCommon(assignmentName: String, assignmentDueAt: String?, expectedGradeLabel: String? = null, assignmentStatus: String? = null, hasCheckPoints : Boolean = false) { | ||
| private fun assertHasAssignmentCommon(assignmentName: String, assignmentDueAt: String?, secondCheckpointDueAt: String? = null, expectedGradeLabel: String? = null, assignmentStatus: String? = null, hasCheckPoints : Boolean = false) { |
There was a problem hiding this comment.
This function has become quite complex with deeply nested conditionals. Consider refactoring into smaller helper functions (e.g., assertCheckpointDueDates, assertSingleAssignmentDueDate) to improve readability and maintainability.
| } | ||
| else { | ||
| if(secondCheckpointDueAt != null) { | ||
| composeTestRule.onAllNodes( |
There was a problem hiding this comment.
The string " 11:59 PM" is hardcoded and duplicated multiple times. Consider extracting it as a constant to avoid duplication and make it easier to maintain.
🧪 Unit Test Results✅ 📱 Student App
✅ 🌅 Horizon
✅ 📦 Submodules
📊 Summary
Last updated: Mon, 12 Jan 2026 14:25:32 GMT |
📊 Code Coverage Report✅ Student
✅ Teacher
|
|
Student Install Page |
Use dataseeding in existing discussion checkpoint tests instead of burnt in course and discussions.
Handle discussions with due dates and no due dates.
Add date converting utility function.
refs: MBL-19655
affects: Student
release note: -