Skip to content

Conversation

@TomTasche
Copy link
Member

#429 and maybe #390

@claude
Copy link

claude bot commented Dec 21, 2025

Claude finished @TomTasche's task —— View job


PR Review: Improve automated tests

Summary

This PR significantly improves the test infrastructure by adding comprehensive UI tests, enhancing core functionality tests, and removing deprecated code. The changes enhance test coverage for document loading, editing, and password-protected documents.

Code Quality and Best Practices ✅

Positive Changes:

  1. Enhanced test structure: The new MainActivityUiTests.java introduces UI automation tests using UiAutomator, complementing the existing Espresso tests
  2. Improved test isolation: Each test properly sets up and tears down activities, preventing test interference
  3. Better error handling: Tests include proper timeouts and detailed assertions with descriptive error messages
  4. Test data management: Static test file extraction in @BeforeClass improves performance

Areas for Improvement:

  1. Code duplication: There's significant duplication between MainActivityTests.java and MainActivityUiTests.java, particularly in helper methods like waitForFirstWord(), getFirstWord(), and evaluateJavascript()
  2. Magic numbers: Hardcoded values like 29665 (port), 10000 (timeout), and 250 (sleep duration) should be extracted to constants
  3. Test method naming: Some test methods could have more descriptive names (e.g., test() in CoreTest.java)

Potential Bugs or Issues ⚠️

  1. Thread safety in CoreTest: The server thread is marked as daemon but might not be properly cleaned up if tests fail
  2. Resource management: Test files are extracted but cleanup might not happen if tests fail catastrophically
  3. Flaky test potential: Tests rely on timing with SystemClock.sleep() which could be unreliable on slower devices

Performance Considerations 🚀

  1. Server initialization: The CoreWrapper server is started/stopped for each test class, which might slow down test execution
  2. File operations: Multiple file copy operations could be optimized using NIO or parallel processing
  3. WebView evaluation: JavaScript evaluation with 10-second timeouts might be excessive for simple operations

Security Concerns 🔒

  1. Hardcoded port: Using a fixed port (29665) could cause conflicts if multiple test instances run simultaneously
  2. Password handling: Test passwords are hardcoded ("passwort") - while acceptable for tests, ensure these don't leak into production
  3. Local server: The CoreWrapper server binds to localhost which is good, but ensure it's properly isolated

Test Coverage 📊

Well-covered areas:

  • Document loading (ODT, PDF, DOCX)
  • Password-protected documents
  • Edit mode functionality
  • Spreadsheet sheet names
  • UI interactions (menu clicks, dialogs)

Missing coverage:

  • Error scenarios beyond password protection
  • Network failures during document loading
  • Concurrent document operations
  • Memory pressure scenarios
  • Different Android API levels

Specific Recommendations 💡

  1. Extract common test utilities: Create a TestUtils class for shared methods between test classes
  2. Improve assertions: Add more specific assertions about document content beyond just the first word
  3. Add parameterized tests: For testing multiple file types with similar logic
  4. Consider using test rules: For common setup/teardown operations
  5. Add integration tests: For the complete document processing pipeline

Notable Code Removal 🗑️

The removal of the parseNative method from core_wrapper.cpp and corresponding Java methods suggests a significant refactoring of the core document parsing logic. This appears to be replaced by the server-based approach using hostFile, which is a cleaner architecture.

Conclusion

This PR substantially improves test coverage and introduces valuable UI testing capabilities. While there are opportunities for reducing code duplication and improving test reliability, the overall changes are positive and will help catch regressions in document handling functionality.

Recommendation: Approve with minor suggestions for improvement.


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.

2 participants