Skip to content

Conversation

@andiwand
Copy link
Member

@andiwand andiwand commented Aug 2, 2025

tests for api 29, 30, 32 do not work after 48d7180

@claude
Copy link

claude bot commented Aug 2, 2025

Claude finished @andiwand's task —— View job


PR Review: Disable Broken Tests

Todo List:

  • Read and analyze the workflow file changes
  • Identify which tests are being disabled
  • Assess the impact and implications
  • Provide comprehensive review feedback

Review Summary

This PR disables Android emulator tests for API levels 29, 30, and 32, leaving only API level 34 tests active in the CI pipeline.

Changes Analysis

The PR comments out three test matrix entries in .github/workflows/build_test.yml:

  • API level 29 (Android 10)
  • API level 30 (Android 11)
  • API level 32 (Android 12L)

Only API level 34 (Android 14) remains active for testing.

Review Feedback

🟡 Concerns

  1. Significant Test Coverage Reduction

    • Disabling tests for API levels 29-32 removes coverage for Android 10, 11, and 12L
    • These API levels represent a substantial portion of active Android devices
    • According to the Target SDK requirement (34) in CLAUDE.md, the app still supports devices down to SDK 23
  2. Lack of Context

    • The PR has no description explaining why these tests are "broken"
    • The comment references commit 48d7180 which appears to be "raise version" - unclear how this relates to test failures
    • No linked issue explaining the root cause or investigation efforts
  3. Technical Debt Accumulation

    • Simply disabling failing tests creates technical debt
    • Without understanding the root cause, the same issues might affect users on these Android versions

🟢 Positive Aspects

  1. Maintains Some Testing

    • At least API level 34 tests are preserved
    • The change is clearly documented with comments
  2. Pragmatic Approach

    • If tests are consistently failing and blocking development, temporarily disabling them can unblock the team

📋 Recommendations

  1. Add PR Description

    • Please explain what specific failures are occurring
    • Include error messages or logs if available
    • Link to any related issues or investigations
  2. Create Tracking Issue

    • File an issue to track the test failures and investigation
    • Set a timeline for re-enabling these tests
    • Consider adding a TODO comment with the issue number
  3. Investigation Before Disabling

    • Have you tried:
      • Checking if it's a timing/flakiness issue?
      • Verifying emulator configuration matches production devices?
      • Running tests locally to reproduce?
      • Checking if the app actually works on these API levels?
  4. Alternative Solutions

    • Consider marking tests as @Ignore with explanations in the test code itself
    • Use fail-fast: false with continue-on-error for specific API levels
    • Implement retry logic for flaky tests
  5. Risk Assessment

    • Ensure the app is still manually tested on Android 10-12L devices
    • Consider if any API-level-specific code paths are now untested

💡 Questions

  1. What specific errors are you seeing on API levels 29-32?
  2. Did these tests work previously? If so, what changed?
  3. Is the app still functional on Android 10-12L devices despite test failures?
  4. Is there a plan to investigate and fix the root cause?

⚠️ Security Considerations

No security issues identified with this change, though reduced test coverage could indirectly lead to undetected security bugs on older Android versions.


While I understand the need to keep CI green, disabling tests should be a temporary measure with a clear plan to restore coverage. Please provide more context about the failures and timeline for resolution.

@andiwand andiwand merged commit 29b5b82 into main Aug 2, 2025
3 checks passed
@andiwand andiwand deleted the disable-broken-tests branch August 2, 2025 14:42
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