[MBL-18125][Student] Add E2E test for custom reminders on assignments with no due date. #3470
Conversation
refs: MBL-18125 affects: Student release note:
![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 adds a new E2E test (testAssignmentNoDueDateCustomReminderE2E) that validates the assignment reminder functionality for assignments without due dates. The test covers important scenarios including adding custom reminders, preventing duplicate reminders, and validating invalid time inputs.
Positive Aspects
- Good test coverage for the no-due-date assignment reminder flow
- Includes edge cases like duplicate reminder prevention and invalid time validation
- Clear logging with step tags makes test execution easy to follow
- Properly structured test with appropriate assertions
Issues Found
-
Calendar object mutation bug (lines 459, 482, 487): The test uses
Calendar.apply { add(...) }which mutates the originalfutureDateobject rather than creating independent instances. This causesreminderDate,reminderDate2, andfutureDateto all reference the same mutated object, leading to unexpected behavior and making the test fragile. -
Unused mutation (line 482):
futureDate.apply { add(Calendar.HOUR, 2) }modifies the object but doesn't assign the result, appearing to be orphaned code. -
Questionable invalid time test case (line 497): Using
00:00(midnight) as the invalid time test case may not be ideal since midnight is typically a valid time. Consider using a truly invalid input or documenting why midnight is invalid for reminders.
Recommendations
- Use
futureDate.clone() as Calendarbefore applying modifications to create independent Calendar instances - Remove or clarify the purpose of the orphaned mutation on line 482
- Consider using a more obviously invalid time input for the invalid time test case
The test logic is sound, but the Calendar handling needs to be fixed to prevent potential flakiness and confusion.
| Log.d(STEP_TAG, "Click on the '+' (Add reminder) button.") | ||
| assignmentReminderPage.clickAddReminder() | ||
|
|
||
| val reminderDate = futureDate.apply { add(Calendar.HOUR, -1) } |
There was a problem hiding this comment.
The futureDate Calendar object is being mutated here with apply { add(Calendar.HOUR, -1) }, but the result is assigned to reminderDate. This doesn't create a new Calendar instance - both variables reference the same object. Consider using futureDate.clone() as Calendar before applying modifications to avoid unintended side effects.
|
|
||
| Log.d(ASSERTION_TAG, "Assert that the custom reminder is not displayed anymore.") | ||
| assignmentReminderPage.assertReminderNotDisplayedWithText(reminderDate.time.toFormattedString()) | ||
| futureDate.apply { add(Calendar.HOUR, 2) } |
There was a problem hiding this comment.
This line mutates futureDate but the result is not used. This appears to be orphaned code that modifies shared state. Since reminderDate was already created from futureDate, this modification affects the original futureDate object unexpectedly. Consider removing this line or clarifying the intent.
| Log.d(STEP_TAG, "Click on the '+' button (Add reminder) to add another custom reminder.") | ||
| assignmentReminderPage.clickAddReminder() | ||
|
|
||
| val reminderDate2 = futureDate.apply { add(Calendar.DAY_OF_MONTH, 1) } |
There was a problem hiding this comment.
Same issue here - reminderDate2 is assigned the result of futureDate.apply { add(Calendar.DAY_OF_MONTH, 1) }, but this mutates the same futureDate object that was already modified on line 482. This creates a chain of mutations that makes the test hard to follow and potentially fragile. Recommend using clone() to create independent Calendar instances.
apps/student/src/androidTest/java/com/instructure/student/ui/e2e/compose/AssignmentsE2ETest.kt
Show resolved
Hide resolved
🧪 Unit Test Results✅ 📱 Student App
✅ 🌅 Horizon
✅ 📦 Submodules
📊 Summary
Last updated: Thu, 08 Jan 2026 14:43:39 GMT |
📊 Code Coverage Report✅ Student
✅ Teacher
✅ Pandautils
📈 Overall Average
|
|
Student Install Page |
|
|
||
| @E2E | ||
| @Test | ||
| @TestMetaData(Priority.NICE_TO_HAVE, FeatureCategory.ASSIGNMENTS, TestCategory.E2E, SecondaryFeatureCategory.ASSIGNMENT_REMINDER) |
There was a problem hiding this comment.
I would put it as 'COMMON' priority because it's not necessarily an edge-case.
Add E2E test for custom reminders on assignments with no due date.
refs: MBL-18125
affects: Student
release note:
Checklist