Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix NOD flaky test #1444

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

fix NOD flaky test #1444

wants to merge 5 commits into from

Conversation

Derrick2000
Copy link

@Derrick2000 Derrick2000 commented Nov 20, 2024

Description

Problem Statement:

  1. Shared Static Variables: The use of static variables like ws (the WebSocketServer instance), port, and countServerDownLatch meant that these resources were shared across multiple test instances.
    This shared state caused interference between tests, especially when they were run multiple times or in parallel, leading to unpredictable failures.

  2. Server Lifecycle Management: The server was started in a BeforeClass method and stopped in an Afterclass method. This approach meant that a single server instance was used for all tests, which could accumulate state or resource locks that affected subsequent tests.

  3. Parameterized Tests with Shared State: Parameterizing the tests without proper isolation led to tests depending on or interfering with each other.
    The combination of shared static variables and parameterization exacerbated the flakiness.

Solution Implemented:

  1. Eliminated Static Variables: Removed the static variables ws, port, and countServerDownLatch.
    By using local variables within the test method runTestScenarioReconnect, we prevent shared state between test runs.

  2. Managed Server Lifecycle Within Test Method: Moved the server initialization and teardown into the runTestScenarioReconnect method. This ensures that each test iteration starts with a fresh server instance and cleans it up afterward.

  3. Removed Parameterization: Eliminated the RunWith(Parameterized.class) annotation and related parameterization code. Instead, used a simple loop within each test method to run the test scenario multiple times (10 iterations), providing the same effect without the complexity and flakiness introduced by parameterization.

Related Issue

Fixes #1443

Motivation and Context

The change eliminates flakiness in the Issue256Test class caused by shared static state, improper resource management, and reliance on parameterization. These issues led to Non-Order-Deterministic (NOD) failures where tests interfered with each other due to shared resources like the WebSocketServer instance and static ports. This change ensures the tests are robust.

How Has This Been Tested?

iDFlakies

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@marci4 marci4 added the Test label Nov 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NOD flaky test
2 participants