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

Remove flaky gRPC tests #3554

Merged
merged 1 commit into from
Jan 29, 2024
Merged

Remove flaky gRPC tests #3554

merged 1 commit into from
Jan 29, 2024

Conversation

joanlopez
Copy link
Contributor

What?

It removes a few gRPC client tests.

Why?

Because by observing them running in CI, and based on what has been discussed in #3545 and #3551, it looks like these have a flaky nature that's not easy to fix, and we even have doubts about its utility, so better to remove them for now.

I also tried to replace the hard-coded certificates, as suggested in #3549, and in a similar way as in #3527, but in this case it's not as easy as that because the server we use for the these tests (HTTPMultiBin) heavily relies on httptest.NewTLSServer from Go stdlib, which manages its own set of certificates.
Indeed, that's the reason why we also have to use the example.com domain there.

So, I'd suggest to either better to discard that idea, or at the very least leave it for the future because it looks like it's going to imply a larger refactor.

Checklist

  • I have performed a self-review of my code.
  • I have added tests for my changes.
  • I have run linter locally (make lint) and all checks pass.
  • I have run tests locally (make tests) and all tests pass.
  • I have commented on my code, particularly in hard-to-understand areas.

Related PR(s)/Issue(s)

I guess it can close #3551

@joanlopez joanlopez requested a review from a team as a code owner January 18, 2024 15:21
@joanlopez joanlopez requested review from mstoykov and removed request for a team January 18, 2024 15:21
@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ef1f214) 72.82% compared to head (c8e6c5e) 72.90%.

❗ Current head c8e6c5e differs from pull request most recent head 452779f. Consider uploading reports for the commit 452779f to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3554      +/-   ##
==========================================
+ Coverage   72.82%   72.90%   +0.08%     
==========================================
  Files         278      278              
  Lines       20947    20944       -3     
==========================================
+ Hits        15254    15269      +15     
+ Misses       4715     4701      -14     
+ Partials      978      974       -4     
Flag Coverage Δ
ubuntu 72.90% <ø> (?)
windows ?

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@joanlopez joanlopez merged commit 40174b7 into grafana:master Jan 29, 2024
25 checks passed
@olegbespalov olegbespalov added this to the v0.50.0 milestone Jan 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix flakyness of the tests in client_test.go
4 participants