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

Increase coverage on otel/app/defaultconfig and otel/app/defaultcomponents #2515

Merged
merged 5 commits into from
Sep 29, 2020

Conversation

joe-elliott
Copy link
Member

Which problem is this PR solving?

Short description of the changes

  • Added the ability to test the factory returned by CreateDefaultConfig()
  • Added the ability to test otelconfigs
  • Refactored Merge() to remove an untested line
  • Increased coverage on defaultscomponents/defaults
jaeger/cmd/opentelemetry/app/defaultconfig (master)$ go test -cover
PASS
coverage: 81.9% of statements
ok  	github.com/jaegertracing/jaeger/cmd/opentelemetry/app/defaultconfig	0.153s

jaeger/cmd/opentelemetry/app/defaultconfig (otel-coverage)$ go test -cover
PASS
coverage: 98.8% of statements
jaeger/cmd/opentelemetry/app/defaultcomponents (master)$ go test -cover
PASS
coverage: 84.6% of statements

jaeger/cmd/opentelemetry/app/defaultcomponents (otel-coverage)$ go test -cover
PASS
coverage: 100.0% of statements

@joe-elliott joe-elliott requested a review from a team as a code owner September 29, 2020 01:07
@joe-elliott joe-elliott changed the title Otel coverage Increased coverage on otel/app/defaultconfig and otel/app/defaultcomponents Sep 29, 2020
@joe-elliott joe-elliott changed the title Increased coverage on otel/app/defaultconfig and otel/app/defaultcomponents Increase coverage on otel/app/defaultconfig and otel/app/defaultcomponents Sep 29, 2020
@codecov
Copy link

codecov bot commented Sep 29, 2020

Codecov Report

Merging #2515 into master will increase coverage by 0.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2515      +/-   ##
==========================================
+ Coverage   95.38%   95.41%   +0.02%     
==========================================
  Files         208      208              
  Lines        9219     9219              
==========================================
+ Hits         8794     8796       +2     
+ Misses        349      348       -1     
+ Partials       76       75       -1     
Impacted Files Coverage Δ
...lugin/sampling/strategystore/adaptive/processor.go 99.07% <0.00%> (-0.93%) ⬇️
plugin/storage/badger/spanstore/reader.go 96.04% <0.00%> (ø)
cmd/query/app/server.go 90.16% <0.00%> (+1.63%) ⬆️
cmd/query/app/static_handler.go 84.40% <0.00%> (+1.83%) ⬆️

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 55d1913...ab78439. Read the comment docs.

Name: "traces",
InputType: configmodels.TracesDataType,
Receivers: []string{"otlp", "jaeger"},
Processors: []string{"batch", "queued_retry"},
Copy link
Member

Choose a reason for hiding this comment

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

hmm qretry should not be there.

Copy link
Member Author

Choose a reason for hiding this comment

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

queued_retry is in here b/c of the missing testdata/addqueuedprocessor.yaml. See my recent push.

Copy link
Member

@pavolloffay pavolloffay left a comment

Choose a reason for hiding this comment

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

The modified test panics when running locally.

Signed-off-by: Joe Elliott <[email protected]>
Signed-off-by: Joe Elliott <[email protected]>
@joe-elliott
Copy link
Member Author

joe-elliott commented Sep 29, 2020

The modified test panics when running locally.

Apologies. I forgot the config file you noticed above. It's now been pushed and I believe tests should pass.

@pavolloffay pavolloffay merged commit 5cd7e0a into jaegertracing:master Sep 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants