feat: introduced retry rules for flaky android push tests

[ttypic]

Added retry rules to prevent flaky android push test fail CI

Summary by CodeRabbit

• New Features
  • Introduced a RetryTestRule to enhance test reliability by allowing tests to be retried upon failure.
  • Integrated the retry mechanism in the AndroidPushTest class, permitting tests to retry up to two times if they fail.
  • Added a cleanup method to ensure a clean state before each test run.
  • Updated the Android testing framework to compile and target SDK version 34, improving compatibility with newer Android features.
  • Modified the testing approach in the GitHub Actions workflow for better execution on the Android emulator, capturing logs on test failures.
  • Updated the version of android-test dependency to improve testing capabilities.

These improvements aim to ensure more consistent test outcomes and reduce false negatives during testing.

[coderabbitai]

Walkthrough

A new class named RetryTestRule has been introduced in the io.ably.lib.test package, implementing the TestRule interface from JUnit. This class allows tests to be retried a specified number of times upon failure. Additionally, the AndroidPushTest class has been updated to utilize this new rule, enabling tests to be retried up to two times if they fail. The overall structure of existing tests remains unchanged, with the retry mechanism enhancing test reliability.

Changes

File Path	Change Summary
android/src/androidTest/java/io/ably/lib/test/RetryTestRule.java	Added new class RetryTestRule implementing TestRule for retrying tests upon failure.
android/src/androidTest/java/io/ably/lib/test/android/AndroidPushTest.java	Added a RetryTestRule instance retryRule initialized with 2 retries, a cleanUp method, and commented out Realtime_push_interface. Annotated WaitingForNewPushDeviceDetails_on_GotPushDeviceDetails with @SdkSuppress(minSdkVersion=21).
.github/workflows/emulate.yml	Updated script execution step to change the method of executing tests on the Android emulator.
android/build.gradle.kts	Changed compileSdk and testOptions.targetSdk from 30 to 34, and set lint.abortOnError to false.
gradle/libs.versions.toml	Updated version of android-test from "0.5" to "1.0.2".

Sequence Diagram(s)

sequenceDiagram
    participant TestRunner
    participant RetryTestRule
    participant Test

    TestRunner->>RetryTestRule: Initialize with retry count (2)
    RetryTestRule->>Test: Execute test
    alt Test fails
        RetryTestRule->>Test: Retry test (1st attempt)
        alt Test fails
            RetryTestRule->>Test: Retry test (2nd attempt)
            alt Test fails
                RetryTestRule->>TestRunner: Throw last exception
            else Test passes
                RetryTestRule->>TestRunner: Return success
            end
        else Test passes
            RetryTestRule->>TestRunner: Return success
        end
    else Test passes
        RetryTestRule->>TestRunner: Return success
    end

Loading

🐇 "In the land of tests, we hop with glee,
Retry our trials, as easy as can be!
With RetryTestRule, we give it a go,
Two chances to shine, let the results flow!
Hooray for the retries, let failures take flight,
In the garden of testing, everything's bright!" 🌼

---

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[coderabbitai]

Caution

Inline review comments failed to post

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (4)

android/src/androidTest/java/io/ably/lib/test/android/AndroidPushTest.java (4)

91-93: LGTM! Consider making retry count configurable.

The addition of the RetryTestRule is a good practice for handling flaky tests. The current implementation with a retry count of 2 is reasonable. However, you might want to consider making this retry count configurable, perhaps through a constant or a configuration parameter. This would allow easier adjustments for different test environments or specific test cases that might require more or fewer retries.

Consider refactoring the retry rule initialization to use a configurable retry count:

-    public RetryTestRule retryRule = new RetryTestRule(2);
+    private static final int RETRY_COUNT = 2;
+    public RetryTestRule retryRule = new RetryTestRule(RETRY_COUNT);

This change would make it easier to adjust the retry count in the future if needed.

---

Line range hint 4-5: Consider addressing disabled tests

There are commented out or disabled test cases in this file. It's generally a good practice to either fix and enable these tests or remove them if they are no longer relevant. Keeping disabled tests in the codebase can lead to confusion and maintenance overhead.

Please review all disabled tests in this file and either:

1. Fix and enable them
2. Remove them if they are no longer needed
3. Add a comment explaining why they are disabled and create an issue to track their resolution

This will help maintain a clean and up-to-date test suite.

---

Line range hint 4-5: Resolve TODO comments

There are TODO comments in the code. While TODOs can be useful for temporary notes, they should be resolved or converted into trackable issues to ensure they're not forgotten.

Please review all TODO comments in this file and either:

1. Implement the required functionality
2. Remove the TODO if it's no longer relevant
3. Convert the TODO into a GitHub issue for proper tracking

This will help ensure that all necessary improvements are properly tracked and implemented.

---

Line range hint 1-1036: Consider splitting the test file

This test file is quite large and contains numerous test cases for different scenarios. While it's comprehensive, it might be beneficial to split it into smaller, more focused test classes. This could improve readability, maintainability, and potentially test execution time if parallel test execution is possible.

Consider refactoring this large test file into smaller, more focused test classes. For example:

• AndroidPushActivationTest
• AndroidPushDeactivationTest
• AndroidPushSubscriptionTest
• AndroidPushErrorHandlingTest

This refactoring would make the test suite more modular and easier to maintain. It would also make it easier to run specific groups of tests when needed.

🛑 Comments failed to post (2)

android/src/androidTest/java/io/ably/lib/test/RetryTestRule.java (2)

36-36: ⚠️ Potential issue

Fix string formatting in error messages

The error messages in System.err.println at lines 36 and 42 use placeholders like ${description.methodName} and $runCount, which won't be interpolated in Java strings. To include variables in the output, use string concatenation or String.format(). Also, $runCount is not defined; you can use (i + 1) to indicate the current run count.

Apply this diff to fix the error messages:

- System.err.println("${description.methodName}: test failed on run: `$runCount`. Will run a maximum of `$timesToRunTestCount` times.");
+ System.err.println(description.getMethodName() + ": test failed on run: " + (i + 1) + ". Will run a maximum of " + timesToRunTestCount + " times.");

- System.err.println("${description.displayName}: giving up after `$timesToRunTestCount` failures");
+ System.err.println(description.getDisplayName() + ": giving up after " + timesToRunTestCount + " failures");

Also applies to: 42-42

---

14-17: ⚠️ Potential issue

Validate times parameter in constructor

Currently, the constructor does not validate the times parameter. If a negative value is passed, it could lead to unexpected behavior. Consider adding a check to ensure times is non-negative.

Apply this diff to add validation:

public RetryTestRule(int times) {
+    if (times < 0) {
+        throw new IllegalArgumentException("times must be non-negative");
+    }
     this.timesToRunTestCount = times + 1;
}

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

    public RetryTestRule(int times) {
        if (times < 0) {
            throw new IllegalArgumentException("times must be non-negative");
        }
        this.timesToRunTestCount = times + 1;
    }

[sacOO7]

I think JUNIT5 supports intelligent retry for failing tests.
junit-team/junit5#3142.
Maybe it's time we should update to JUNIT5

[sacOO7]

Not sure if Junit5 really added native support for retries, but surely some hacks available out there junit-team/junit5#1558 (comment)

[ttypic]

@sacOO7 yes, junit 5 indeed has retry support out of the box, but unfortunately Android runner doesn't support junit 5. Here is issue for this https://issuetracker.google.com/issues/127100532

[sacOO7]

@sacOO7 yes, junit 5 indeed has retry support out of the box, but unfortunately Android runner doesn't support junit 5. Here is issue for this https://issuetracker.google.com/issues/127100532

Sure, let's just make sure existing retry script is stable enough to handle proper retries. Also, there are lot of retry scripts available online -> junit-team/junit5#1558 (comment)

[coderabbitai]

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (2)

.github/workflows/emulate.yml (2)

38-38: Approved: Enhanced logging for better debugging.

Adding the --info flag to the Gradle command is a good improvement. It will provide more detailed output during the test execution, which can be crucial for debugging issues in the CI environment, especially for flaky tests.

Consider using the --info flag conditionally. You could set up a GitHub Actions input variable to control the verbosity level, allowing for more flexibility:

inputs:
  gradle-log-level:
    description: 'Gradle log level (quiet, lifecycle, info, debug)'
    required: false
    default: 'lifecycle'

steps:
  - uses: reactivecircus/android-emulator-runner@v2
    with:
      api-level: ${{ matrix.android-api-level }}
      script: ./gradlew :android:connectedAndroidTest --${{ inputs.gradle-log-level }}

This way, you can easily adjust the log level without modifying the workflow file, which can be useful for debugging specific runs without permanently increasing log verbosity.

---

Line range hint 1-45: Overall improvement in debugging capabilities.

The addition of the --info flag to the Gradle command aligns well with the PR objectives of addressing flaky Android push tests. This change enhances the workflow's ability to provide detailed information during test execution, which is crucial for identifying and resolving intermittent failures.

To further improve the handling of flaky tests, consider implementing a mechanism to track and report on test flakiness over time. This could involve:

1. Parsing the Gradle output to identify flaky tests.
2. Storing the results of flaky tests in a separate artifact or database.
3. Creating a custom GitHub Action or script to analyze and report on test flakiness trends.

This approach would provide valuable insights into which tests are consistently problematic and help prioritize test stability efforts.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 3d017ab and 4b13635.

📒 Files selected for processing (4)

• .github/workflows/emulate.yml (1 hunks)
• android/build.gradle.kts (1 hunks)
• android/src/androidTest/java/io/ably/lib/test/RetryTestRule.java (1 hunks)
• android/src/androidTest/java/io/ably/lib/test/android/AndroidPushTest.java (5 hunks)

🚧 Files skipped from review as they are similar to previous changes (2)

• android/src/androidTest/java/io/ably/lib/test/RetryTestRule.java
• android/src/androidTest/java/io/ably/lib/test/android/AndroidPushTest.java

🔇 Additional comments (1)

android/build.gradle.kts (1)

Line range hint 34-36: Reconsider disabling lint errors

Setting abortOnError = false in the lint configuration prevents the build from failing due to lint errors. While this can be useful in certain CI/CD scenarios, it might hide important issues that should be addressed.

Consider the following alternatives:

1. Instead of disabling all lint errors, you could selectively ignore specific lint issues that are known to be false positives or not applicable to your project. This can be done using the disable or ignore options in the lint configuration.

2. If this change is intended to address specific CI/CD pipeline issues, it might be better to configure the CI tool to continue despite lint errors, rather than modifying the project's build configuration.

3. If you decide to keep this configuration, ensure that lint errors are still being reported and reviewed regularly, perhaps as part of your code review process.

To help make an informed decision, let's check if there are any current lint issues in the project:

This script will help determine if there are current lint issues that need addressing, which can guide the decision on whether to keep abortOnError = false.

✅ Verification successful

Lint Configuration Verification

No lint issues currently found in the project. Disabling lint errors with abortOnError = false is not hiding any existing issues.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for lint issues in the project

# Run lint and capture the output
./gradlew lint > lint_output.txt 2>&1

# Check if there are any errors or warnings
if grep -E "Error:|Warning:" lint_output.txt; then
  echo "Lint issues found. Consider addressing these before disabling lint errors."
else
  echo "No lint issues found. Disabling lint errors might not be necessary."
fi

# Clean up
rm lint_output.txt

Length of output: 226

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 4e28154 and 11cbdca.

📒 Files selected for processing (2)

• android/src/androidTest/java/io/ably/lib/test/android/AndroidPushTest.java (4 hunks)
• android/src/main/AndroidManifest.xml (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• android/src/androidTest/java/io/ably/lib/test/android/AndroidPushTest.java

[coderabbitai]

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (2)

android/src/androidTest/java/io/ably/lib/test/android/AndroidPushTest.java (2)

91-93: LGTM! Consider extracting the retry count to a constant.

The addition of the RetryTestRule is a good approach to handle flaky tests, which is particularly useful for push notification testing. This aligns well with the PR objective of enhancing test reliability.

Consider extracting the retry count (2) to a private static final constant at the class level. This would make it easier to adjust the retry count in the future if needed. For example:

private static final int RETRY_COUNT = 2;

@Rule
public RetryTestRule retryRule = new RetryTestRule(RETRY_COUNT);

---

Line range hint 1-1843: Consider cleanup and potential refactoring of the test suite.

While reviewing the file, I noticed a few areas that could be improved:

1. There are some TODO comments in the code (e.g., line 4). It would be good to address these or create issues to track them if they can't be resolved immediately.

2. There's a commented-out test method Realtime_push_interface (around line 1443). If this test is no longer needed, consider removing it. If it's still relevant, please uncomment and update it as necessary.

3. Given the size and complexity of this test suite, it might be beneficial to consider splitting it into smaller, more focused test classes. This could improve readability and maintainability.

Could you please review these suggestions and let me know your thoughts on potentially refactoring the test suite?

#!/bin/bash
# Find TODO comments and commented-out methods
echo "TODO comments:"
rg "TODO" AndroidPushTest.java
echo "Commented-out methods:"
rg "//\s*@Test" AndroidPushTest.java

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between c450b20 and bb21435.

📒 Files selected for processing (5)

• .github/workflows/emulate.yml (1 hunks)
• android/build.gradle.kts (2 hunks)
• android/src/androidTest/java/io/ably/lib/test/RetryTestRule.java (1 hunks)
• android/src/androidTest/java/io/ably/lib/test/android/AndroidPushTest.java (6 hunks)
• gradle/libs.versions.toml (1 hunks)

🚧 Files skipped from review as they are similar to previous changes (4)

• .github/workflows/emulate.yml
• android/build.gradle.kts
• android/src/androidTest/java/io/ably/lib/test/RetryTestRule.java
• gradle/libs.versions.toml

🔇 Additional comments (2)

android/src/androidTest/java/io/ably/lib/test/android/AndroidPushTest.java (2)

Line range hint 1-1843: Summary of AndroidPushTest.java review

Overall, the changes to this file are positive and align with the PR objective of enhancing the reliability of Android push tests. The addition of the RetryTestRule should help manage flaky tests, and the @SdkSuppress annotation ensures that certain tests run on appropriate Android versions.

However, there are a few points that could use attention:

1. Consider extracting the retry count to a constant for easier maintenance.
2. Clarification is needed on the reason for the SDK version restriction in the WaitingForNewPushDeviceDetails_on_GotPushDeviceDetails test.
3. Some cleanup of TODO comments and commented-out code would be beneficial.
4. Given the size and complexity of this test suite, consider refactoring it into smaller, more focused test classes in the future.

Please review these suggestions and let me know if you need any further clarification or have any questions.

---

983-983: LGTM! Please clarify the reason for SDK version restriction.

The addition of @SdkSuppress(minSdkVersion = 21) to the WaitingForNewPushDeviceDetails_on_GotPushDeviceDetails test is approved. This ensures the test runs only on Android 5.0 (Lollipop) and above.

Could you please clarify the specific reason for restricting this test to Android 5.0 and above? Is there a particular push notification feature or behavior that's only available or reliable from this version onwards?

Also, if the library still supports Android versions below 5.0, how is the functionality tested for those versions?

✅ Verification successful

Verified! The @SdkSuppress(minSdkVersion = 21) annotation is correctly applied to WaitingForNewPushDeviceDetails_on_GotPushDeviceDetails, and there are no other tests for this functionality without SDK restrictions.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check if there are any other tests for the same functionality without SDK restrictions
rg --type java "WaitingForNewPushDeviceDetails.*GotPushDeviceDetails" -g '!AndroidPushTest.java'

Length of output: 98

---

Script:

#!/bin/bash
# Search for the specific test method across the entire codebase
rg --type java "WaitingForNewPushDeviceDetails_on_GotPushDeviceDetails"

# Search within AndroidPushTest.java for any conditional SDK version checks
rg --type java "@SdkSuppress" android/src/androidTest/java/io/ably/lib/test/android/AndroidPushTest.java

Length of output: 417

[sacOO7]

LGTM since tests are passing