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

Address sucker_punch flaky test #4344

Merged
merged 7 commits into from
Feb 10, 2025

Conversation

sarahchen6
Copy link
Contributor

@sarahchen6 sarahchen6 commented Feb 4, 2025

What does this PR do?
This PR avoids a concurrency issue in sucker_punch tests by retrieving the length of the span arrays.

Motivation:
Minimize our flaky tests! Per Ivo's comment below, the issue was caused by traces.collect(&:spans) in the original fetch_spans call. The rspec thread was reading the span arrays in TraceSegment#spans at the same time as the sucker_punch thread was mutating them. This created flakiness with occasional concurrency issues. Since the sucker_punch tests only require span counts, we can instead specifically retrieve the array lengths, which avoids reading (and iterating) through the span arrays like before.

This PR addresses: https://github.com/DataDog/ruby-guild/issues/186.

Change log entry
No.

Additional Notes:
N/A

How to test the change?
Hope to never see this bug again in CI... I tested the flakiness by running the test 1000 times in a row without failure.

@sarahchen6 sarahchen6 changed the title Attempt to address sucker_punch flaky test Address sucker_punch flaky test Feb 4, 2025
@datadog-datadog-prod-us1
Copy link
Contributor

datadog-datadog-prod-us1 bot commented Feb 4, 2025

Datadog Report

Branch report: sarahchen6/attempt_suckerpunch_flaky_test
Commit report: e02ad49
Test service: dd-trace-rb

✅ 0 Failed, 22011 Passed, 1476 Skipped, 6m 23s Total Time

@pr-commenter
Copy link

pr-commenter bot commented Feb 4, 2025

Benchmarks

Benchmark execution time: 2025-02-07 16:32:19

Comparing candidate commit e02ad49 in PR branch sarahchen6/attempt_suckerpunch_flaky_test with baseline commit 6b6ec18 in branch master.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 31 metrics, 2 unstable metrics.

@sarahchen6 sarahchen6 force-pushed the sarahchen6/attempt_suckerpunch_flaky_test branch from f5df59f to 7d4066c Compare February 4, 2025 23:13
@sarahchen6 sarahchen6 force-pushed the sarahchen6/attempt_suckerpunch_flaky_test branch 2 times, most recently from 90a3346 to 9aa6492 Compare February 5, 2025 23:01
@sarahchen6 sarahchen6 force-pushed the sarahchen6/attempt_suckerpunch_flaky_test branch from 6e9867e to d1767ee Compare February 6, 2025 20:40
@github-actions github-actions bot added the dev/testing Involves testing processes (e.g. RSpec) label Feb 6, 2025
@sarahchen6 sarahchen6 force-pushed the sarahchen6/attempt_suckerpunch_flaky_test branch from d1767ee to 98e5299 Compare February 6, 2025 20:56
@codecov-commenter
Copy link

codecov-commenter commented Feb 6, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.72%. Comparing base (6b6ec18) to head (e02ad49).

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4344      +/-   ##
==========================================
- Coverage   97.73%   97.72%   -0.02%     
==========================================
  Files        1347     1347              
  Lines       82349    82351       +2     
  Branches     4187     4187              
==========================================
- Hits        80486    80478       -8     
- Misses       1863     1873      +10     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@sarahchen6 sarahchen6 requested review from ivoanjo and marcotc February 7, 2025 02:01
Copy link
Member

@ivoanjo ivoanjo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable! Any reason for this to still be marked as draft? :)

@sarahchen6 sarahchen6 force-pushed the sarahchen6/attempt_suckerpunch_flaky_test branch from 1dff58c to e02ad49 Compare February 7, 2025 16:08
@sarahchen6 sarahchen6 marked this pull request as ready for review February 7, 2025 16:09
@sarahchen6 sarahchen6 requested a review from a team as a code owner February 7, 2025 16:09
@sarahchen6 sarahchen6 requested a review from ivoanjo February 7, 2025 16:09
Copy link
Member

@ivoanjo ivoanjo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 LGTM. Ahaha one of those PRs that looks deceptively simple after all the work we poured into making it simple :D

Copy link
Member

@ivoanjo ivoanjo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 LGTM. Ahaha one of those PRs that looks deceptively simple after all the work we poured into making it simple :D

@sarahchen6 sarahchen6 merged commit ba51201 into master Feb 10, 2025
494 checks passed
@sarahchen6 sarahchen6 deleted the sarahchen6/attempt_suckerpunch_flaky_test branch February 10, 2025 18:17
@github-actions github-actions bot added this to the 2.11.0 milestone Feb 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dev/testing Involves testing processes (e.g. RSpec)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants