[MBL-19944] Learner Dashboard - Add E2E Tests#4022
[MBL-19944] Learner Dashboard - Add E2E Tests#4022suhaibabsi-inst wants to merge 3 commits intomasterfrom
Conversation
BuildsNo apps were built for this pull request. To trigger app builds, include a line starting with Commit: Address review comments (99dea33) |
There was a problem hiding this comment.
Review: Learner Dashboard E2E Tests
This PR adds UI test infrastructure and 6 E2E test scenarios for the new Learner Dashboard feature. The structure is well-organised — widget-scoped nested structs keep the helper readable, the waitUntil(.visible) / waitUntil(.vanish) pattern is applied consistently, and the tests cover a sensible set of user journeys (widget visibility, settings toggle, grade pill, global announcements).
Issues found
-
Label-based element lookups in
LearnerDashboardHelper(3 locations) —showGradesToggle(line 28),widgetToggle(title:)(line 31), andcourseCard(courseName:)(line 44) all locate elements by visible text rather than accessibility identifiers. This couples tests to English strings and will produce false failures if labels change or are localised. See inline comments for suggested ID-based alternatives. -
Inconsistent accessibility ID prefix on
feedbackButton(line 27) — Uses"Dashboard.Settings.*"while the surrounding elements use"DashboardSettings.*". One of these doesn't match the production view; worth confirming and aligning. -
Missing
typeparameter onnewDashboardToggle(line 26) — Every other element in the helper passes an explicit XCUIElement type tofind(id:). Omitting it here makes the lookup less precise. -
Unnecessary
String → Floatconversion intestCourseCardGradeVisibility(lines 115–119) —pointsPossibleis declared as"10"(aString) and then passed asFloat(pointsPossible), which yieldsFloat?. Using a numeric literal directly is clearer and avoids the implicit optional. See inline comment.
Positive notes
- The per-item ID pattern in
AnnouncementsWidget("…GlobalAnnouncement.Id.\(announcement.id)") is a great model — it's the approach I'd recommend applying tocourseCardas well. testGlobalAnnouncementShownAndDismissedcorrectly posts the announcement after login and usespullToRefresh+waitUntil(.visible)to handle timing — good defensive practice.testToggleCoursesWidgetOffHidesWidgetverifies both the toggle state change and the downstream UI effect, which is exactly the right scope for this kind of test.
PR ChecksCommit: Address review comments (99dea33)
|
There was a problem hiding this comment.
PR Review Update — Synchronize
Re-reviewed the latest commits on this PR.
Note: this update is posted as a review comment rather than an issue comment because the issue-comment tool isn't available in this run. The workflow allowlist references
mcp__github__create_or_update_issue_comment, but that tool isn't exposed by the configured GitHub MCP server, and the availableadd_issue_commentrequires a permission grant that isn't available in this headless run.
Progress checklist
- Previously identified issues that have been resolved — cannot enumerate; the previous review summary couldn't be fetched (the read-side tools —
get_pull_request_reviews/get_issue_comments— aren't in this run's allowlist). - Previously identified issues still present — same caveat.
- New issues found in this update — none blocking.
Summary of what's in the current diff
This PR adds E2E test coverage and accessibility identifiers for the new Learner Dashboard:
LearnerDashboardHelper.swift— newBaseHelpernamespace mirroring widget structure (Settings, Courses, Announcements, WeeklySummary, ToDo, Invitations) with aSettings.WidgetIDenum whose raw values match the productionconfig.id.rawValue.- Production identifier additions:
Dashboard.Settings.widgetToggle.<id>,Dashboard.Settings.showGradesToggle, and a per-course suffix onDashboard.Courses.CourseCard.cardButton.<id>. GlobalAnnouncementsWidgetViewnow interpolatescardViewModel.accessibilityId(a stableString) instead ofcardViewModel.id(the whole model). NewaccessibilityId: String { model.id }onGlobalAnnouncementCardViewModel— this is a real correctness improvement, since the previous interpolation embedded a struct's default description into the identifier.StudentAppDelegateforce-enablesisLearnerDashboardEnabledOnInstancefor UI tests. Verified safe:StudentTabBarController.swift:111-114still gates the new dashboard onExperimentalFeature.revertToOldStudentDashboard, soDashboardTestsopting into[.revertToOldStudentDashboard]still routes to the old dashboard as intended.LearnerDashboardTests.swift— six new E2E tests covering courses widget, settings entry, all-courses navigation, widget toggle hide, grade pill visibility, and global announcement details flow.
Notes (not blocking)
LearnerDashboardTests.swift:119—Float(pointsPossible)is a redundant cast (pointsPossibleis alreadyFloat).LearnerDashboardTests.swift:121—String(pointsPossible)forFloat = 10produces"10.0", not"10". The seeder appears to accept either, butString(Int(pointsPossible))would be more obviously what's intended.testGlobalAnnouncementShownAndDismissedseeds a course/enrollment that doesn't appear to be exercised by the assertions — the test could likely be tightened, though boilerplate parity with the other tests has its own value.
Overall: clean, well-scoped change. No correctness or security concerns found in this update.
PR Info
refs: MBL-19944
builds: none
affects: none
release note: none
Test Plan
LearnerDashboard E2E Tests should to pass