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

Added test on plugin/sampling/strategystore/adaptive/options #5105

Merged
merged 5 commits into from
Jan 18, 2024

Conversation

tarhphilomina
Copy link
Contributor

@tarhphilomina tarhphilomina commented Jan 15, 2024

Which problem is this PR solving?

Part of #5068

Description of the changes

  • Added test to adaptive sampling plugin options

How was this change tested?

  • make test

Checklist

Signed-off-by: tarhphilomina <156124227+tarhphilomina@users.noreply.github.com>
Signed-off-by: tarhphilomina <156124227+tarhphilomina@users.noreply.github.com>
@tarhphilomina tarhphilomina marked this pull request as ready for review January 15, 2024 07:24
@tarhphilomina tarhphilomina requested a review from a team as a code owner January 15, 2024 07:24
Comment on lines 1 to 13
// Copyright (c) 2018 The Jaeger Authors.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.
Copy link
Member

Choose a reason for hiding this comment

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

remove this header and run make fmt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @yurishkuro , I removed the header as instructed but the make fmt command seems to be revealing errors on files I did not edit. Please is this the expected behavior of the make fmt command ?

image

Copy link
Member

Choose a reason for hiding this comment

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

seems your toolchain may not be up to date: Go 1.21 and make install-tools

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. The problem was , I had two different go versions installed.

assert.Equal(t, 1e-5, opts.MinSamplingProbability)
assert.Equal(t, (1.0 / float64(time.Minute/time.Second)), opts.MinSamplesPerSecond)
assert.Equal(t, 5*time.Second, opts.LeaderLeaseRefreshInterval)
assert.Equal(t, 60*time.Second, opts.FollowerLeaseRefreshInterval)
Copy link
Member

Choose a reason for hiding this comment

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

I suggest using defaultXX constants here. What is the point of testing for exact values? If we change the default constant this test will fail, which provides no value.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right , I have excluded the TestFlagDefaults test.

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.

tagging as unresolved

Signed-off-by: tarhphilomina <156124227+tarhphilomina@users.noreply.github.com>
@yurishkuro yurishkuro added the changelog:test Change that's adding missing tests or correcting existing tests label Jan 16, 2024
Copy link

github-actions bot commented Jan 16, 2024

Test Results

2 061 tests  +1   2 051 ✅ +1   1m 8s ⏱️ ±0s
  216 suites ±0      10 💤 ±0 
    1 files   ±0       0 ❌ ±0 

Results for commit a54379b. ± Comparison against base commit 663a04e.

♻️ This comment has been updated with latest results.

Signed-off-by: tarhphilomina <156124227+tarhphilomina@users.noreply.github.com>
@tarhphilomina
Copy link
Contributor Author

Please have a look at the PR , it should be working perfectly.
image

Copy link

codecov bot commented Jan 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (663a04e) 95.56% compared to head (a54379b) 95.55%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5105      +/-   ##
==========================================
- Coverage   95.56%   95.55%   -0.02%     
==========================================
  Files         317      317              
  Lines       18284    18284              
==========================================
- Hits        17474    17471       -3     
- Misses        651      653       +2     
- Partials      159      160       +1     
Flag Coverage Δ
cassandra-3.x 25.58% <ø> (ø)
cassandra-4.x 25.58% <ø> (ø)
elasticsearch-5.x 19.87% <ø> (ø)
elasticsearch-6.x 19.87% <ø> (+0.01%) ⬆️
elasticsearch-7.x 19.99% <ø> (-0.02%) ⬇️
elasticsearch-8.x 20.09% <ø> (ø)
grpc-badger 19.51% <ø> (-0.02%) ⬇️
kafka 14.09% <ø> (ø)
opensearch-1.x 20.00% <ø> (ø)
opensearch-2.x 19.99% <ø> (-0.02%) ⬇️
unittests 93.21% <ø> (-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.

@yurishkuro yurishkuro merged commit 26579f0 into jaegertracing:main Jan 18, 2024
39 checks passed
@yurishkuro
Copy link
Member

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

2 participants