Skip to content

Conversation

@mahadzaryab1
Copy link
Collaborator

@mahadzaryab1 mahadzaryab1 commented Nov 23, 2024

Which problem is this PR solving?

Description of the changes

  • We had a bug report come in [Bug]: query: TLS handshake error #6230 which wasn't caught by our unit tests. The reason here was because the test was not checking the status code of the response received from the server but rather just the error returned from doing Client.Do.
    • This PR also cleans up the tests to remove the manual dial call and an unnecessary filter in the test body.

How was this change tested?

TestServerHTTPTLS/should_pass_with_TLS_client_to_trusted_TLS_server_with_correct_hostname
TestServerHTTPTLS/should_pass_with_TLS_client_with_cert_to_trusted_TLS_server_requiring_cert
TestServerHTTPTLS/should_pass_with_TLS_client_with_cert_to_trusted_TLS_HTTP_server_requiring_cert_and_insecure_GRPC_server

Checklist

Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
Signed-off-by: Mahad Zaryab <[email protected]>
@mahadzaryab1 mahadzaryab1 requested a review from a team as a code owner November 23, 2024 00:11
@mahadzaryab1 mahadzaryab1 requested a review from jkowall November 23, 2024 00:11
@mahadzaryab1 mahadzaryab1 added the changelog:test Change that's adding missing tests or correcting existing tests label Nov 23, 2024
@dosubot dosubot bot added the bug label Nov 23, 2024
@codecov
Copy link

codecov bot commented Nov 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.44%. Comparing base (edb896e) to head (a33f7e8).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6241      +/-   ##
==========================================
+ Coverage   96.42%   96.44%   +0.02%     
==========================================
  Files         355      355              
  Lines       20149    20149              
==========================================
+ Hits        19428    19433       +5     
+ Misses        532      528       -4     
+ Partials      189      188       -1     
Flag Coverage Δ
badger_v1 8.31% <ø> (ø)
badger_v2 1.67% <ø> (ø)
cassandra-4.x-v1 14.39% <ø> (ø)
cassandra-4.x-v2 1.61% <ø> (ø)
cassandra-5.x-v1 14.39% <ø> (ø)
cassandra-5.x-v2 1.61% <ø> (ø)
elasticsearch-6.x-v1 18.59% <ø> (-0.01%) ⬇️
elasticsearch-7.x-v1 18.67% <ø> (ø)
elasticsearch-8.x-v1 18.85% <ø> (+<0.01%) ⬆️
elasticsearch-8.x-v2 1.67% <ø> (+<0.01%) ⬆️
grpc_v1 9.44% <ø> (ø)
grpc_v2 6.98% <ø> (ø)
kafka-v1 8.88% <ø> (ø)
kafka-v2 1.67% <ø> (ø)
memory_v2 1.66% <ø> (ø)
opensearch-1.x-v1 18.73% <ø> (ø)
opensearch-2.x-v1 18.73% <ø> (+<0.01%) ⬆️
opensearch-2.x-v2 1.66% <ø> (ø)
tailsampling-processor 0.46% <ø> (ø)
unittests 95.35% <ø> (+0.02%) ⬆️

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.


🚨 Try these New Features:

Signed-off-by: Mahad Zaryab <[email protected]>
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

Thanks!

@yurishkuro yurishkuro merged commit 7b4631c into jaegertracing:main Nov 23, 2024
52 checks passed
@mahadzaryab1 mahadzaryab1 deleted the fix-tls-unit-tests branch November 23, 2024 01:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug changelog:test Change that's adding missing tests or correcting existing tests

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: query: TLS handshake error

2 participants