[MBL-19712][Teacher] Extend Profile Settings E2E tests with empty name field and profile picture change#3523
Conversation
…or and profile picture change flow with crop functionality. refs: MBL-19712 affects: Teacher 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 two new E2E tests for the Profile Settings functionality in the Teacher app:
- Testing empty profile name handling (verification that clearing the name field preserves the previous value)
- Testing profile picture change from gallery
Positive aspects:
- Good test coverage for profile settings edge cases
- Well-structured test methods with clear logging for each step
- Proper use of assertions to verify expected behavior
- Good reuse of page object pattern with new helper methods
Issues to address:
-
Exception handling workaround (SettingsE2ETest.kt:221, 285): The try-catch blocks catching
IllegalStateExceptionappear to be workarounds for flaky navigation. Consider extracting this into a helper method or using more robust waiting/navigation strategies. -
Intents lifecycle management (SettingsE2ETest.kt:267):
Intents.init()andIntents.release()should ideally be managed at test lifecycle level using@Before/@AfterorIntentsRulefor better resource management. -
Assertion mechanism (EditProfileSettingsPage.kt:75): Using raw
assert()in Android instrumentation tests is problematic as assertions can be disabled at runtime. Use Espresso's assertion mechanisms or proper assertion libraries instead. -
Null safety (TeacherTest.kt:196): Missing null check for
externalCacheDirwhich could cause issues if the cache directory is unavailable. Should handle this case gracefully.
Minor observations:
- The
stubImagePickerIntent()method is a good addition for testing file selection flows - The new page object methods follow existing patterns well
- Test logging is comprehensive and helpful for debugging
Overall, this is a solid addition to the test suite. The main concerns are around test reliability and proper assertion usage. Once these are addressed, this will be ready to merge.
apps/teacher/src/androidTest/java/com/instructure/teacher/ui/e2e/classic/SettingsE2ETest.kt
Show resolved
Hide resolved
apps/teacher/src/androidTest/java/com/instructure/teacher/ui/e2e/classic/SettingsE2ETest.kt
Show resolved
Hide resolved
...her/src/androidTest/java/com/instructure/teacher/ui/pages/classic/EditProfileSettingsPage.kt
Show resolved
Hide resolved
| fun stubImagePickerIntent(fileName: String) { | ||
| val resultData = Intent() | ||
| val dir = activityRule.activity.externalCacheDir | ||
| val file = File(dir?.path, fileName) |
There was a problem hiding this comment.
Missing null safety check. If externalCacheDir is null, file will be created with a null path, which could lead to unexpected behavior or crashes. Consider:
val dir = activityRule.activity.externalCacheDir
?: throw IllegalStateException("External cache directory not available")
val file = File(dir.path, fileName)refs: MBL-19712 affects: Teacher release note:
🧪 Unit Test Results✅ 📱 Teacher App
✅ 📦 Submodules
📊 Summary
Last updated: Thu, 12 Feb 2026 15:52:15 GMT |
📊 Code Coverage Report✅ Student
✅ Teacher
✅ Pandautils
📈 Overall Average
|
|
Teacher Install Page |
| import com.instructure.espresso.page.withId | ||
| import com.instructure.espresso.typeText | ||
| import com.instructure.teacher.R | ||
| import com.instructure.pandautils.R as PandaUtilsR |
There was a problem hiding this comment.
Why do we need to import an other R file? As far as I know resources and ids are merged after build.
3 participants
Summary
Extend Profile Settings E2E tests to validate empty name field behavior and profile picture change flow with crop functionality.
refs: MBL-19712
affects: Teacher
release note:
Checklist