Skip to content
This repository has been archived by the owner on Jul 1, 2022. It is now read-only.

Production and test code cleanup #664

Merged
merged 3 commits into from Oct 18, 2019

Conversation

esukram
Copy link
Contributor

@esukram esukram commented Oct 18, 2019

Which problem is this PR solving?

None

Short description of the changes

During the work of #653 I saw some code that was generating warnings in the IDE. This PR does solve these.

Please let me know, what you think.

Several unused variables, resource leaks or other minor glitches in test classes are fixed.

Signed-off-by: Markus Heimbach <markus@esukram.org>
The used depricated constructor just consumed the given parameter and
did not use it at all. Thus is is safe to use the default constructor.

Signed-off-by: Markus Heimbach <markus@esukram.org>
An unused import remained in the class. Just removed it.

Signed-off-by: Markus Heimbach <markus@esukram.org>
@esukram
Copy link
Contributor Author

esukram commented Oct 18, 2019

One thing that made me wonder in particular:
JaegerSpanTest.java has three test methods that have testConcurrentModification in its name. But as far I can tell, the concurrence is not tested at all.
I did not change that in this PR to not modify the test behaviour, but I would be interested in your opinion.

Like in here:

@codecov
Copy link

codecov bot commented Oct 18, 2019

Codecov Report

Merging #664 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #664   +/-   ##
=========================================
  Coverage     89.59%   89.59%           
  Complexity      567      567           
=========================================
  Files            69       69           
  Lines          2086     2086           
  Branches        266      266           
=========================================
  Hits           1869     1869           
  Misses          135      135           
  Partials         82       82
Impacted Files Coverage Δ Complexity Δ
...ing/internal/samplers/RemoteControlledSampler.java 91.25% <ø> (ø) 16 <0> (ø) ⬇️

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 27d7fce...847b3c0. Read the comment docs.

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 for the PR

@yurishkuro
Copy link
Member

testConcurrentModification happens because the tests are changing the same collection (conceptully) that they are iterating over.

@yurishkuro yurishkuro merged commit 46798bd into jaegertracing:master Oct 18, 2019
@esukram esukram deleted the code-cleanup branch October 18, 2019 16:14
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants