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

Additional Test Coverage around TimelineViewingLayer #617

Merged
merged 3 commits into from
Aug 22, 2020

Conversation

tklever
Copy link
Contributor

@tklever tklever commented Aug 13, 2020

Which problem is this PR solving?

  • Adding additional test coverage around TimelineViewingLayer
  • Removing some "randomness" from the test suite that caused some coverage jitter (see: TimelineViewingLayer showing up in many Codecov reports in PR's that don't touch this component)
  • Some minor refactors inside TimelineViewingLayer

Short description of the changes

  • Added tests for more unit coverage
  • Added a new function to DRY out the component a little (_getAnchorAndShift)
  • Refactored the ref to use CreateRef API introduced in React 16.3 over callback ref
  • Removed randomness from test suite causing code coverage fluxuations based on random chance (executing out of bounds paths)

In addition to rounding out the test suite, this commit also removes some of the "Math.random" calls from the test suite. These randoms were causing some code coverage jitter in the test suite by randomly executing certain code paths. Removing these randomizations will keep coverage reporting consistent and avoid random "coverage losses" based on chance.

Signed-off-by: Tim Klever <Tim.V.Klever@aexp.com>
CreateRef API was introduced in React 16.3

Signed-off-by: Tim Klever <Tim.V.Klever@aexp.com>
@codecov
Copy link

codecov bot commented Aug 13, 2020

Codecov Report

Merging #617 into master will increase coverage by 0.10%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #617      +/-   ##
==========================================
+ Coverage   93.44%   93.54%   +0.10%     
==========================================
  Files         227      227              
  Lines        5902     5901       -1     
  Branches     1486     1485       -1     
==========================================
+ Hits         5515     5520       +5     
+ Misses        346      340       -6     
  Partials       41       41              
Impacted Files Coverage Δ
...eViewer/TimelineHeaderRow/TimelineViewingLayer.tsx 100.00% <100.00%> (+8.47%) ⬆️
...ui/src/utils/DraggableManager/DraggableManager.tsx 96.93% <0.00%> (+1.02%) ⬆️

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 60ee1fd...3bc82ef. Read the comment docs.

@yurishkuro
Copy link
Member

@rubenvp8510 rubenvp8510 self-requested a review as a code owner August 22, 2020 01:10
@rubenvp8510 rubenvp8510 self-assigned this Aug 22, 2020
Copy link
Collaborator

@rubenvp8510 rubenvp8510 left a comment

Choose a reason for hiding this comment

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

This looks good.

@rubenvp8510 rubenvp8510 merged commit 6513b53 into jaegertracing:master Aug 22, 2020
@yurishkuro
Copy link
Member

@rubenvp8510 please make sure PRs (and, respectively, merged commits) have the title that satisfies "good commit message" criteria (see https://github.com/jaegertracing/jaeger-ui/blob/master/CONTRIBUTING.md#making-a-change)

@tklever tklever deleted the TimelineViewingLayer-tests branch August 23, 2020 06:30
vvvprabhakar pushed a commit to vvvprabhakar/jaeger-ui that referenced this pull request Jul 4, 2021
* add additional tests around existing TimelineViewingLayer functionality

In addition to rounding out the test suite, this commit also removes some of the "Math.random" calls from the test suite. These randoms were causing some code coverage jitter in the test suite by randomly executing certain code paths. Removing these randomizations will keep coverage reporting consistent and avoid random "coverage losses" based on chance.

Signed-off-by: Tim Klever <Tim.V.Klever@aexp.com>

* migrate from callback ref to CreateRef in TimelineViewingLayer

CreateRef API was introduced in React 16.3

Signed-off-by: Tim Klever <Tim.V.Klever@aexp.com>

Co-authored-by: Ruben Vargas Palma <ruben.vp8510@gmail.com>
Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>
vvvprabhakar pushed a commit to vvvprabhakar/jaeger-ui that referenced this pull request Jul 4, 2021
* add additional tests around existing TimelineViewingLayer functionality

In addition to rounding out the test suite, this commit also removes some of the "Math.random" calls from the test suite. These randoms were causing some code coverage jitter in the test suite by randomly executing certain code paths. Removing these randomizations will keep coverage reporting consistent and avoid random "coverage losses" based on chance.

Signed-off-by: Tim Klever <Tim.V.Klever@aexp.com>

* migrate from callback ref to CreateRef in TimelineViewingLayer

CreateRef API was introduced in React 16.3

Signed-off-by: Tim Klever <Tim.V.Klever@aexp.com>

Co-authored-by: Ruben Vargas Palma <ruben.vp8510@gmail.com>
Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>
vvvprabhakar pushed a commit to vvvprabhakar/jaeger-ui that referenced this pull request Jul 5, 2021
* add additional tests around existing TimelineViewingLayer functionality

In addition to rounding out the test suite, this commit also removes some of the "Math.random" calls from the test suite. These randoms were causing some code coverage jitter in the test suite by randomly executing certain code paths. Removing these randomizations will keep coverage reporting consistent and avoid random "coverage losses" based on chance.

Signed-off-by: Tim Klever <Tim.V.Klever@aexp.com>

* migrate from callback ref to CreateRef in TimelineViewingLayer

CreateRef API was introduced in React 16.3

Signed-off-by: Tim Klever <Tim.V.Klever@aexp.com>

Co-authored-by: Ruben Vargas Palma <ruben.vp8510@gmail.com>
Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>
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