Add automated tests for password-protected ODT files

[TomTasche]

• Add password-test.odt test asset with password "passwort"
• Add CoreTest methods for native password handling validation
• Add MainActivityTests method for UI password dialog testing
• Tests cover wrong password, correct password, and no password scenarios
• Validates both core C++ functionality and Android UI workflow

Fixes #396

🤖 Generated with Claude Code

[claude]

Claude finished @TomTasche's task —— View job

---

Code Review for Password-Protected ODT File Tests

I've completed my review of the pull request that adds automated tests for password-protected ODT files.

Todo List:

• Read and analyze all changed files
• Review code quality and best practices
• Check for potential bugs or issues
• Assess performance considerations
• Evaluate security concerns
• Analyze test coverage
• Provide comprehensive feedback

✅ Code Quality and Best Practices

Strengths:

• Well-structured test organization with proper setup/teardown methods
• Clear and descriptive test method names
• Good use of @LargeTest annotations for UI tests
• Consistent error code validation (-2 for password errors, 0 for success)
• Proper resource cleanup in @After and @AfterClass methods
• Excellent CI/CD debugging support with logcat capture and artifact uploads

Suggestions for Improvement:

1. Magic Numbers: Consider extracting error codes to constants:

   private static final int SUCCESS = 0;
   private static final int ERROR_PASSWORD_REQUIRED = -2;

2. Hardcoded File Size: The file size assertion at MainActivityTests.java:231 could be fragile:

   Assert.assertEquals("password-test.odt file size mismatch", 12671L, testFile.length());

   Consider removing this check or making it more flexible.

3. Extract Test Password: The password "passwort" is hardcoded in multiple places. Consider:

   private static final String TEST_PASSWORD = "passwort";

🐛 Potential Bugs

1. Critical Bug in CoreTest.java:91: Incorrect variable reference in assertion:

   CoreWrapper.CoreResult result = CoreWrapper.backtranslate(coreOptions, htmlDiff);
   Assert.assertEquals(0, coreResult.errorCode); // Bug: should be 'result.errorCode'

   This checks the wrong result object and could cause tests to pass incorrectly.

2. Missing Directory Cleanup: Test output directories (output_password_test) are created but never cleaned up, which could accumulate over multiple test runs.

⚡ Performance Considerations

• Good: Files are extracted once in @BeforeClass rather than per test
• Good: Parallel test matrix in CI for different API levels
• Thread.sleep Usage: The 10-second sleep in testPDF() is concerning but appears to be for CI stability

🔒 Security Concerns

1. Test Password in Repository: While acceptable for test files, consider:

   • Adding a comment clarifying this is test-only data
   • Using a more obvious test password like "test123"

2. Password Input Type: The test types passwords in plain text. Ensure the actual password dialog uses inputType="textPassword" for masking.

📊 Test Coverage

Well Covered:

• ✅ No password provided (expects error)
• ✅ Wrong password (expects error)
• ✅ Correct password (expects success)
• ✅ Both native C++ layer and Android UI layer
• ✅ Password retry flow

Missing Coverage:

• ❌ Canceling the password dialog
• ❌ Empty password submission
• ❌ Very long passwords or special characters
• ❌ Multiple password attempts limit (if applicable)

📝 Additional Observations

1. CI/CD Enhancements: The workflow changes adding logcat capture and artifact uploads are excellent for debugging flaky tests

2. Version Bump: AndroidManifest.xml shows version changes (194→195, 3.39→3.40) which seems appropriate for a new feature

3. Native Code: The C++ password handling implementation looks correct with proper exception handling

4. Test Isolation: Good practice extracting test files to a dedicated directory to avoid conflicts

Summary

This is a solid implementation of password-protected document testing. The main issue is the bug in CoreTest.java line 91 that needs fixing. The test coverage is good for basic scenarios, and the CI/CD enhancements will help with debugging. Consider the suggested improvements for code maintainability.

[claude]

Claude finished @TomTasche's task —— View job

---

Code Review for Password-Protected ODT Tests

I've completed my review of this pull request that adds automated tests for password-protected ODT files. Here's my feedback:

Todo List:

• Review CI/CD workflow changes in build_test.yml
• Examine test asset (password-test.odt)
• Analyze CoreTest.java additions for native password handling
• Review MainActivityTests.java for UI password dialog testing
• Review PasswordTestIsolated.java
• Assess overall test coverage and provide feedback

✅ Code Quality and Best Practices

Strengths:

• Good test structure with proper setup/teardown methods
• Clear test naming that describes what each test validates
• Appropriate use of @LargeTest annotation for UI tests
• Consistent error code checking (-2 for password errors, 0 for success)
• Proper resource cleanup in @After and @AfterClass methods
• Excellent CI debugging support with extensive logging and artifact collection

Suggestions:

1. Magic Numbers: Consider defining constants for error codes instead of using -2 and 0 directly:

   private static final int ERROR_CODE_SUCCESS = 0;
   private static final int ERROR_CODE_PASSWORD_REQUIRED = -2;

2. File Size Assertion: The hardcoded file size checks in MainActivityTests.java:226 and PasswordTestIsolated.java:90 could be fragile:

   Assert.assertEquals("password-test.odt file size mismatch", 12671L, testFile.length());

   Consider removing this check or making it more flexible with a range.

🐛 Potential Bugs or Issues

1. Error Code Check Bug in CoreTest.java:91:

   CoreWrapper.CoreResult result = CoreWrapper.backtranslate(coreOptions, htmlDiff);
   Assert.assertEquals(0, coreResult.errorCode); // Should be 'result.errorCode'

   This checks the wrong variable - should check result.errorCode instead of coreResult.errorCode.

2. Missing Directory Cleanup: The output directories created in tests (output_password_test) are not cleaned up after tests complete.

3. Hardcoded Password: The password "passwort" is hardcoded in multiple places. Consider extracting it to a constant or test configuration.

⚡ Performance Considerations

1. CI/CD Improvements: The new logcat capturing and artifact uploads are excellent for debugging CI failures
2. Test Isolation: Good use of separate output directories for each test to avoid conflicts
3. File Extraction: Files are extracted once in @BeforeClass rather than for each test, which is efficient
4. Thread.sleep Usage: While not ideal, the sleep calls appear necessary for CI stability. Consider using more robust synchronization if possible.

🔒 Security Concerns

1. Password in Repository: The test password "passwort" is visible in the code. While this is just test data, consider:

   • Adding a comment explaining this is test-only data
   • Using a more obvious test password like "test123" to make it clear it's not production

2. No Password Masking Verification: The UI test types the password in plain text. This is acceptable for tests, but ensure the actual password field in the app uses proper input masking (inputType="textPassword").

📊 Test Coverage

Good Coverage:

• ✅ No password scenario (expects error)
• ✅ Wrong password scenario (expects error)
• ✅ Correct password scenario (expects success)
• ✅ Both native (CoreTest) and UI (MainActivityTests) layers tested
• ✅ Password retry flow in UI tests
• ✅ Isolated test for debugging CI issues

Missing Coverage:

• ❌ No test for canceling the password dialog
• ❌ No test for empty password submission
• ❌ No test for very long passwords or special characters
• ❌ No test for password-protected files with different encryption methods (if supported)

📝 Additional Recommendations

1. Documentation: Consider adding a comment in the test files explaining the test asset's password for future maintainers

2. Test Data: The PR description mentions the password but this could be documented in the code as well

3. CI Debugging: The extensive logging and artifact collection added to the workflow is excellent for troubleshooting

4. Test Stability: The PasswordTestIsolated class shows good practices for dealing with flaky CI tests through isolation and extensive logging

Overall, this is a solid implementation of password-protected document testing. The main issues are the typo in the error code check and missing cleanup of test directories. The test coverage is good for the basic scenarios, though additional edge cases could be considered in future iterations.