[MBL-19440][Student] Extend Notifications E2E test to verify inbox conversations are excluded

[adamNagy56]

Summary

Extends testNotificationsE2E to verify that inbox conversations are not displayed in the Notifications page.

refs: MBL-19440
affects: Student
release note:

Checklist

• Run E2E test suite

[claude]

Code Review for PR #3339

Summary

This PR extends the testNotificationsE2E test to verify that inbox conversations are not displayed in the Notifications page. The implementation is sound and follows existing patterns in the codebase.

---

✅ Positive Aspects

1. Follows Existing Patterns: The code adheres to the established testing patterns in the repository, using appropriate logging tags (PREPARATION_TAG, STEP_TAG, ASSERTION_TAG) and the page object pattern.

2. Good Test Coverage: The test properly verifies both positive (conversation appears in Inbox) and negative (conversation does NOT appear in Notifications) cases, which is excellent for E2E testing.

3. Proper API Usage: Uses the ConversationsApi.createConversation() correctly to seed test data.

4. Clean Page Object Implementation: The new assertNotificationNotDisplayed() method in NotificationPage.kt is well-implemented with proper Espresso matchers.

---

🔍 Issues & Recommendations

1. Hard-coded Sleep Duration (apps/student/src/androidTest/java/com/instructure/student/ui/e2e/classic/NotificationsE2ETest.kt:122)

Issue: Using sleep(3000) is a test anti-pattern that makes tests slower and potentially flaky.

sleep(3000) //wait here to be sure that the conversation notification does not appears.

Recommendation: Consider using a more robust polling mechanism similar to what's already used elsewhere in the test (lines 82-93 and 126-144). However, I understand this may be acceptable for E2E tests when verifying negative assertions (that something does NOT appear). If this approach is intentional, consider adding a more detailed comment explaining why a fixed wait is necessary here.

Alternative approach:

// Poll to ensure conversation doesn't appear (negative assertion)
var conversationAppeared = false
repeat(5) {
    try {
        refresh()
        sleep(500)
        notificationPage.assertNotificationDisplayed(seededConversation.subject)
        conversationAppeared = true
        return@repeat
    } catch (e: AssertionError) {
        // Expected - conversation should not appear
    }
}
if (conversationAppeared) {
    throw AssertionError("Conversation should not appear in notifications")
}

2. Import Organization (apps/student/src/androidTest/java/com/instructure/student/ui/pages/classic/NotificationPage.kt:21)

Minor: The doesNotExist import was added but NoMatchingViewException on line 20 appears unused in the file after your changes. Consider verifying if this import is still needed.

---

🎯 Code Quality Assessment

Aspect	Rating	Notes
Code Quality	✅ Good	Follows project conventions and style
Best Practices	⚠️ Minor Issues	Hard-coded sleep could be improved
Potential Bugs	✅ None Found	Logic is sound
Performance	⚠️ Minor	3-second sleep adds test time
Security	✅ N/A	No security concerns for test code
Test Coverage	✅ Excellent	Tests both positive and negative cases

---

📋 Pre-Merge Checklist

As noted in the PR description:

• Run E2E test suite - Please ensure this test passes consistently in the E2E suite before merging

---

💡 Additional Notes

The implementation is well thought out and integrates seamlessly with the existing test structure. The use of refresh and navigation between Inbox and Notifications pages appropriately validates the requirement. The page object method assertNotificationNotDisplayed() is reusable and well-scoped.

Overall Assessment: ✅ Approved with minor suggestions

The code is ready to merge once the E2E tests pass. The suggested improvements are optional and would only marginally improve the test reliability.

[github-actions]

📊 Code Coverage Report

✅ Student

• PR Coverage: 44.08%
• Master Coverage: 44.08%
• Delta: +0.00%

✅ Teacher

• PR Coverage: 25.60%
• Master Coverage: 25.60%
• Delta: +0.00%

✅ Pandautils

• PR Coverage: 22.51%
• Master Coverage: 22.50%
• Delta: +0.00%

---

📈 Overall Average

• PR Coverage: 30.73%
• Master Coverage: 30.73%
• Delta: +0.00%

[inst-danger]

Student Install Page

[inst-danger]

Student Install Page

[claude]

Review Summary

This PR adds test coverage to verify that inbox conversations do NOT appear in the notifications list, which is a good addition to the E2E test suite.

Strengths

✅ Follows existing page object pattern with the new assertNotificationNotDisplayed() method
✅ Good use of existing test infrastructure (ConversationsApi, logging patterns)
✅ Clear test intent with descriptive assertion messages

Key Issues

⚠️ Hard-coded sleep: The sleep(3000) on line 121 is a testing anti-pattern that can cause flakiness and unnecessary delays. Please use Espresso's idling resources or explicit wait conditions instead.

⚠️ Defensive coding: Consider adding null checks and using .first() instead of array indexing when accessing seeded data.

Suggestions

💡 The test flow navigates to Inbox to verify the conversation exists before checking Notifications. Consider whether this intermediate step is necessary or if a comment explaining its purpose would help.

💡 Consider adding KDoc to the new page object method for clarity on when to use negative assertions.

Overall, this is a solid test addition. The main concern is the hard-coded sleep which should be addressed to prevent test flakiness.

[inst-danger]

Student Install Page

[inst-danger]

Student Install Page