KEMBAR78
chore(fix): unflake MetricsTracerTest by kolea2 · Pull Request #313 · googleapis/java-bigtable · GitHub
Skip to content

Conversation

@kolea2
Copy link
Contributor

@kolea2 kolea2 commented May 18, 2020

Fixes #310

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label May 18, 2020
@codecov
Copy link

codecov bot commented May 18, 2020

Codecov Report

Merging #313 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #313   +/-   ##
=========================================
  Coverage     80.69%   80.69%           
  Complexity     1049     1049           
=========================================
  Files            99       99           
  Lines          6541     6541           
  Branches        344      344           
=========================================
  Hits           5278     5278           
  Misses         1082     1082           
  Partials        181      181           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 50a19a6...b667001. Read the comment docs.

@kolea2 kolea2 marked this pull request as ready for review May 18, 2020 20:53
@kolea2 kolea2 requested a review from igorbernstein2 May 18, 2020 20:53
@kolea2 kolea2 marked this pull request as draft May 18, 2020 21:12
@igorbernstein2
Copy link
Contributor

I dont think this will fix the flakiness. I think the issue is that there is a race between the server thread (the emulator) and the client recording the duration:

client: starts 2 timers (first response, overall op) and sends request
server: sleeps(beforeSleep), sends response

race starts now
server: sleeps (afterSleep)
client: stops first response timer

server: finishes the call
client: stops overall timer

if the client won the race then timer will not be greater than overall timer - afterSleep, however if the server won the race then this condition will not be true.

I think the easiest fix is to add some slop to:
assertThat(firstRowLatency).isIn(Range.closed(beforeSleep, elapsed - afterSleep));

something like:
assertThat(firstRowLatency).isIn(Range.closed(beforeSleep, elapsed - afterSleep/2));

@kolea2 kolea2 marked this pull request as ready for review May 26, 2020 16:40
Copy link
Contributor

@igorbernstein2 igorbernstein2 left a comment

Choose a reason for hiding this comment

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

lgtm

@kolea2 kolea2 merged commit 29da6de into googleapis:master May 27, 2020
@kolea2 kolea2 deleted the metrics-test branch May 27, 2020 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Investigate flaky MetricsTracerTest

3 participants