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

Add integration test for ErrTraceNotFound #2478

Merged
merged 4 commits into from
Sep 16, 2020

Conversation

yurishkuro
Copy link
Member

@yurishkuro yurishkuro commented Sep 15, 2020

This is especially interesting when using go-plugin based storage, where the error has to be correctly passed through gRPC client.

The test revealed that Badger did not return NotFound when expected, so it's fixed here as well.

I added documentation to the span Reader API with expectations about not-found.

Signed-off-by: Yuri Shkuro <[email protected]>
Copy link
Contributor

@objectiser objectiser left a comment

Choose a reason for hiding this comment

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

LGTM - although the title is a bit misleading, as the integration test was already testing for ErrTraceNotFound, so assume it wasn't being run against badger? But need to fix TestFindNothing before merging.

@jpkrohling
Copy link
Contributor

This is related to #2471, isn't it?

Copy link
Contributor

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

I'm blocking this for now, because of the test failure. Looks like the same is being addressed in another PR already, so, we might want to close this.

Signed-off-by: Yuri Shkuro <[email protected]>
@yurishkuro
Copy link
Member Author

although the title is a bit misleading, as the integration test was already testing for ErrTraceNotFound, so assume it wasn't being run against badger

@objectiser where do you see integration test checking for NotFound? Badger implementation clearly did not satisfy the API contract for that (which I also fix in this PR), yet it was passing integration tests, so I assume there was no explicit test for NotFound.

@jpkrohling I don't know if #2471 is fixing the same issue (and it's doing it incorrectly anyway, findTraces can return empty list).

Signed-off-by: Yuri Shkuro <[email protected]>
@objectiser
Copy link
Contributor

@yurishkuro yep, lgtm.

@yurishkuro
Copy link
Member Author

I added documentation to the span Reader API with expectations about not-found.

@codecov
Copy link

codecov bot commented Sep 15, 2020

Codecov Report

Merging #2478 into master will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2478      +/-   ##
==========================================
+ Coverage   95.52%   95.53%   +0.01%     
==========================================
  Files         208      208              
  Lines       10750    10756       +6     
==========================================
+ Hits        10269    10276       +7     
  Misses        405      405              
+ Partials       76       75       -1     
Impacted Files Coverage Δ
plugin/storage/badger/spanstore/reader.go 96.79% <100.00%> (ø)
plugin/storage/integration/integration.go 81.39% <100.00%> (+1.01%) ⬆️
...lugin/sampling/strategystore/adaptive/processor.go 99.20% <0.00%> (-0.80%) ⬇️
cmd/query/app/server.go 90.69% <0.00%> (+1.55%) ⬆️

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 0a889e8...d0194aa. Read the comment docs.

@jpkrohling
Copy link
Contributor

Thanks for documenting the interface!

@jpkrohling jpkrohling merged commit 37eaced into jaegertracing:master Sep 16, 2020
IvanBodnya pushed a commit to IvanBodnya/jaeger that referenced this pull request Sep 17, 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.

3 participants