Skip to content

[MBL-19944] Learner Dashboard - Add E2E Tests#4022

Draft
suhaibabsi-inst wants to merge 3 commits intomasterfrom
chore/MBL-19944-LearnerDashboard-E2E-Tests
Draft

[MBL-19944] Learner Dashboard - Add E2E Tests#4022
suhaibabsi-inst wants to merge 3 commits intomasterfrom
chore/MBL-19944-LearnerDashboard-E2E-Tests

Conversation

@suhaibabsi-inst
Copy link
Copy Markdown
Contributor

@suhaibabsi-inst suhaibabsi-inst commented May 3, 2026

PR Info

refs: MBL-19944
builds: none
affects: none
release note: none

Test Plan

LearnerDashboard E2E Tests should to pass

@suhaibabsi-inst suhaibabsi-inst self-assigned this May 3, 2026
@bitrise
Copy link
Copy Markdown
Contributor

bitrise Bot commented May 3, 2026

Builds

No apps were built for this pull request.

To trigger app builds, include a line starting with builds: followed by app names (Student, Teacher, Parent, or All) in your pull request's message.

Commit: Address review comments (99dea33)
Built At: May 06 11:08 CEST (05/06 03:08 AM MDT)

Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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), and courseCard(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 type parameter on newDashboardToggle (line 26) — Every other element in the helper passes an explicit XCUIElement type to find(id:). Omitting it here makes the lookup less precise.

  • Unnecessary String → Float conversion in testCourseCardGradeVisibility (lines 115–119) — pointsPossible is declared as "10" (a String) and then passed as Float(pointsPossible), which yields Float?. 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 to courseCard as well.
  • testGlobalAnnouncementShownAndDismissed correctly posts the announcement after login and uses pullToRefresh + waitUntil(.visible) to handle timing — good defensive practice.
  • testToggleCoursesWidgetOffHidesWidget verifies both the toggle state change and the downstream UI effect, which is exactly the right scope for this kind of test.

Comment thread Core/TestsFoundation/UITestHelperNamespaces/LearnerDashboardHelper.swift Outdated
Comment thread Core/TestsFoundation/UITestHelperNamespaces/LearnerDashboardHelper.swift Outdated
Comment thread Core/TestsFoundation/UITestHelperNamespaces/LearnerDashboardHelper.swift Outdated
Comment thread Student/StudentE2ETests/LearnerDashboard/LearnerDashboardTests.swift Outdated
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 3, 2026

PR Checks

Commit: Address review comments (99dea33)
Built At: May 06 09:26 UTC (05/06 03:26 AM MDT)

✅ PR Description
✅ Copyright Headers
✅ SwiftLint
✅ Build CITests
✅ Unit Tests
✅ Code Coverage

Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 available add_issue_comment requires 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 — new BaseHelper namespace mirroring widget structure (Settings, Courses, Announcements, WeeklySummary, ToDo, Invitations) with a Settings.WidgetID enum whose raw values match the production config.id.rawValue.
  • Production identifier additions: Dashboard.Settings.widgetToggle.<id>, Dashboard.Settings.showGradesToggle, and a per-course suffix on Dashboard.Courses.CourseCard.cardButton.<id>.
  • GlobalAnnouncementsWidgetView now interpolates cardViewModel.accessibilityId (a stable String) instead of cardViewModel.id (the whole model). New accessibilityId: String { model.id } on GlobalAnnouncementCardViewModel — this is a real correctness improvement, since the previous interpolation embedded a struct's default description into the identifier.
  • StudentAppDelegate force-enables isLearnerDashboardEnabledOnInstance for UI tests. Verified safe: StudentTabBarController.swift:111-114 still gates the new dashboard on ExperimentalFeature.revertToOldStudentDashboard, so DashboardTests opting 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:119Float(pointsPossible) is a redundant cast (pointsPossible is already Float).
  • LearnerDashboardTests.swift:121String(pointsPossible) for Float = 10 produces "10.0", not "10". The seeder appears to accept either, but String(Int(pointsPossible)) would be more obviously what's intended.
  • testGlobalAnnouncementShownAndDismissed seeds 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant