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.

Sign up for GitHub

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

Jump to bottom

Merging to release-5.3.9: Fixing Race Condition in CI (#6761) #6763

Merged
merged 1 commit into from
Dec 12, 2024
Merged

Merging to release-5.3.9: Fixing Race Condition in CI (#6761) #6763

merged 1 commit into from
Dec 12, 2024

Conversation

buger
Copy link
Member

@buger buger commented Dec 12, 2024 •
edited by github-actions bot
Loading

User description

Fixing Race Condition in CI (#6761)

User description

Fixing a unit test with a race condition that was already fixed in
master branch.

PR Type

Bug fix, Tests

Description

  • Fixed a race condition in the TestStartPubSubHandler unit test by
    replacing a boolean flag with a buffered channel to track callback
    invocation.
  • Introduced a timeout mechanism using context.WithTimeout to ensure
    the callback is executed within a specific time frame.
  • Removed arbitrary sleep and replaced it with a deterministic approach
    using select and context for better test reliability.
  • Improved the overall robustness and reliability of the test.

Changes walkthrough 📝

Relevant files
Bug fix
redis_cluster_test.go
Fix race condition in PubSub handler test                               

storage/redis_cluster_test.go

  • Replaced a boolean flag with a buffered channel to track callback
    invocation.
  • Added a timeout mechanism using context.WithTimeout to ensure callback
    execution is verified within a specific time.
  • Removed arbitrary sleep and replaced it with a more deterministic
    approach using select and context.
  • Improved test reliability by ensuring callback execution is properly
    awaited.
    		•  +12/-4   

    💡 PR-Agent usage: Comment /help "your question" on any pull
    request to receive relevant information

    PR Type

    Bug fix, Tests

    Description

    • Fixed a race condition in the TestStartPubSubHandler unit test by replacing a boolean flag with a buffered channel to track callback invocation.
    • Introduced a timeout mechanism using context.WithTimeout to ensure the callback is executed within a specific time frame.
    • Removed arbitrary sleep and replaced it with a deterministic approach using select and context for better test reliability.
    • Improved the overall robustness and reliability of the test.

    Changes walkthrough 📝

    Relevant files
    Bug fix
    redis_cluster_test.go
    Fix race condition in PubSub handler test                               

    storage/redis_cluster_test.go

  • Replaced a boolean flag with a buffered channel to track callback
    invocation.
  • Introduced a timeout mechanism using context.WithTimeout to ensure
    callback execution is verified within a specific time.
  • Removed arbitrary sleep and replaced it with a deterministic approach
    using select and context.
  • Improved test reliability by ensuring callback execution is properly
    awaited.
    		•  +12/-4   

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    @mativm02
    Fixing Race Condition in CI (#6761)
    a8b26c1 
    ### **User description**
Fixing a unit test with a race condition that was already fixed in
`master` branch.


___

### **PR Type**
Bug fix, Tests


___

### **Description**
- Fixed a race condition in the `TestStartPubSubHandler` unit test by
replacing a boolean flag with a buffered channel to track callback
invocation.
- Introduced a timeout mechanism using `context.WithTimeout` to ensure
the callback is executed within a specific time frame.
- Removed arbitrary sleep and replaced it with a deterministic approach
using `select` and context for better test reliability.
- Improved the overall robustness and reliability of the test.



___



### **Changes walkthrough** 📝
<table><thead><tr><th></th><th align="left">Relevant
files</th></tr></thead><tbody><tr><td><strong>Bug
fix</strong></td><td><table>
<tr>
  <td>
    <details>
<summary><strong>redis_cluster_test.go</strong><dd><code>Fix race
condition in PubSub handler test</code>&nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; </dd></summary>
<hr>

storage/redis_cluster_test.go

<li>Replaced a boolean flag with a buffered channel to track callback
<br>invocation.<br> <li> Added a timeout mechanism using
<code>context.WithTimeout</code> to ensure callback <br>execution is
verified within a specific time.<br> <li> Removed arbitrary sleep and
replaced it with a more deterministic <br>approach using
<code>select</code> and context.<br> <li> Improved test reliability by
ensuring callback execution is properly <br>awaited.<br>


</details>


  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6761/files#diff-6881545acab41fff3221454e0efb16302c696ea1217da926f80332a62ef51c71">+12/-4</a>&nbsp;
&nbsp; </td>

</tr>
</table></td></tr></tr></tbody></table>

___

> 💡 **PR-Agent usage**: Comment `/help "your question"` on any pull
request to receive relevant information

(cherry picked from commit 4e7a120)
    @buger buger enabled auto-merge (squash) December 12, 2024 08:46
    @github-actions GitHub Actions
    Copy link
    Contributor

    API Changes

    no api changes detected

    @github-actions GitHub Actions
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    6761 - Fully compliant

    Fully compliant requirements:

    • Fix a race condition in the TestStartPubSubHandler unit test.
    • Replace a boolean flag with a buffered channel to track callback invocation.
    • Introduce a timeout mechanism using context.WithTimeout to ensure callback execution is verified within a specific time.
    • Remove arbitrary sleep and replace it with a deterministic approach using select and context.
    • Improve the overall robustness and reliability of the test.

    Not compliant requirements:
    None

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Possible Bug
    Ensure that the context.WithTimeout duration (100ms) is sufficient for the callback to be invoked in all environments, as it might cause flaky tests in slower systems.

    @github-actions GitHub Actions
    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    Extend the timeout duration to prevent test failures in slower environments

    Increase the timeout duration in the context to ensure the test does not fail due to
    slow execution in certain environments.

    storage/redis_cluster_test.go [2074]

    -ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond)
+ctx, cancel := context.WithTimeout(context.Background(), 500*time.Millisecond)
    Suggestion importance[1-10]: 8

    Why: Increasing the timeout duration is a valid and impactful suggestion to ensure the test does not fail in slower environments. This change improves the robustness of the test without altering its core functionality.

    		8

    @sonarqubecloud SonarQubeCloud
    Copy link

    Quality Gate Passed Quality Gate passed

    Issues
    0 New issues
    0 Accepted issues

    Measures
    0 Security Hotspots
    0.0% Coverage on New Code
    0.0% Duplication on New Code

    See analysis details on SonarQube Cloud

    @buger buger merged commit bb67972 into release-5.3.9 Dec 12, 2024
    33 of 38 checks passed
    @buger buger deleted the merge/release-5.3.9/4e7a120d007778cadd47c509957b5b9df5dcd3ce branch December 12, 2024 09:00
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers
    No reviews
    Assignees
    No one assigned
    Labels
    Review effort [1-5]: 2
    Projects
    None yet
    Milestone
    No milestone
    Development

    Successfully merging this pull request may close these issues.
    2 participants
    @buger @mativm02