[MBL-19326][Teacher] Refactor testFilesE2E test and add comment's attachment file #3471
Conversation
refs: MBL-19326 affects: Teacher release note:
refs: MBL-19326 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 enhances the E2E test for file attachments in SpeedGrader by adding comment attachment testing functionality. The overall approach is solid, but there are several areas that need attention for improved test reliability and code quality.
Positive Aspects
✅ Good test coverage expansion - Adding comment attachment tests fills an important gap in the test suite
✅ New FileChooserPage object - Well-structured page object with clear, reusable methods
✅ Proper logging - Comprehensive logging throughout the test for debugging
✅ Assertion improvements - Good use of comment count assertions to verify state changes
Issues Found
- File cleanup missing (FilesE2ETest.kt:163-177) - The PDF file created in Downloads is never deleted, causing test pollution
- Thread.sleep anti-pattern (FilesE2ETest.kt:215) - Hardcoded 5-second sleep makes tests slow and unreliable; use proper wait conditions instead
- Brittle file picker navigation (FilesE2ETest.kt:188-210) - UIAutomator logic relies on device-specific UI text that may vary across Android versions
- Resource leak potential (FilesE2ETest.kt:163-177) - PDF document creation doesn't use
useblocks, risking resource leaks if exceptions occur - Storage location concerns (FilesE2ETest.kt:163) - Using DIRECTORY_DOWNLOADS may require runtime permissions and behaves inconsistently across devices
- Missing newline (FileChooserPage.kt:102) - File should end with a newline per Kotlin style guide
Recommendations
- Refactor file handling: Move test files to app-specific directories using
getExternalFilesDir()to avoid permission issues - Add cleanup: Implement an
@Aftermethod or try-finally block to ensure test files are deleted - Replace Thread.sleep: Use Espresso IdlingResource or explicit wait conditions
- Extract file picker logic: Create a reusable helper method for UIAutomator file selection with better error handling
- Use proper resource management: Wrap PdfDocument and FileOutputStream in
useblocks
Test Coverage Note
The test currently validates:
- Comment attachment display
- Comment count updates
- File upload workflow
Consider adding negative test cases:
- Upload cancellation
- Invalid file types
- Network failure scenarios
Overall, this is a valuable addition to the test suite that improves comment attachment coverage. With the issues addressed, it will be much more reliable across different devices and Android versions.
apps/teacher/src/androidTest/java/com/instructure/teacher/ui/e2e/classic/FilesE2ETest.kt
Show resolved
Hide resolved
apps/teacher/src/androidTest/java/com/instructure/teacher/ui/e2e/classic/FilesE2ETest.kt
Show resolved
Hide resolved
|
|
||
| Log.d(STEP_TAG, "Select 'Device' as file source.") | ||
| fileChooserPage.chooseDevice() | ||
|
|
There was a problem hiding this comment.
Complex UIAutomator navigation logic: This navigation fallback logic is brittle and relies on specific UI text and descriptions that may vary across Android versions and device manufacturers. Consider:
- Using more robust selectors or abstracting this into a reusable helper method
- Adding error handling and logging if the file cannot be found
- Consider using a more reliable file selection approach, perhaps by placing test files in a known location or using content URIs
There was a problem hiding this comment.
As it is written here, isn't there a better method to find and select a file from the file system? This seems a bit fragile like this. I am fine with this as long as there isn't any better solution that can be easily implemented, but it would be worth to check it in a timebox (max 1-2 hours).
apps/teacher/src/androidTest/java/com/instructure/teacher/ui/pages/classic/FileChooserPage.kt
Show resolved
Hide resolved
apps/teacher/src/androidTest/java/com/instructure/teacher/ui/e2e/classic/FilesE2ETest.kt
Show resolved
Hide resolved
📊 Code Coverage Report✅ Student
✅ Teacher
✅ Pandautils
📈 Overall Average
|
🧪 Unit Test Results✅ 📱 Teacher App
✅ 📦 Submodules
📊 Summary
Last updated: Thu, 08 Jan 2026 15:16:29 GMT |
|
Teacher Install Page |
| @@ -0,0 +1,102 @@ | |||
| /* | |||
| * Copyright (C) 2025 - present Instructure, Inc. | |||
There was a problem hiding this comment.
year is 2026 here... :D
| pdfFile.createNewFile() | ||
|
|
||
| Log.d(PREPARATION_TAG, "Write content to PDF file '${pdfFile.name}'.") | ||
| android.graphics.pdf.PdfDocument().apply { |
There was a problem hiding this comment.
you u remove the android.graphics.pdf part and import it?
There was a problem hiding this comment.
Also there are some other classes/methods which can be imported, please review and change them to import if it's possible
|
|
||
| Log.d(STEP_TAG, "Select 'Device' as file source.") | ||
| fileChooserPage.chooseDevice() | ||
|
|
There was a problem hiding this comment.
As it is written here, isn't there a better method to find and select a file from the file system? This seems a bit fragile like this. I am fine with this as long as there isn't any better solution that can be easily implemented, but it would be worth to check it in a timebox (max 1-2 hours).
Adds test case for attaching PDF file to SpeedGrader comments in FilesE2E test.
refs: MBL-19326
affects: Teacher
release note:
Checklist