Skip to content

Issue #428: Smart Flank - print how accurate test times are#436

Merged
Macarse merged 1 commit into
masterfrom
macarse/issue428
Jan 15, 2019
Merged

Issue #428: Smart Flank - print how accurate test times are#436
Macarse merged 1 commit into
masterfrom
macarse/issue428

Conversation

@Macarse
Copy link
Copy Markdown
Contributor

@Macarse Macarse commented Dec 11, 2018

Opening this for feedback.

Example output:

Shard times: 10s, 20s, 30s
3 tests / 3 shards

Actual shard times: 9s (-11%), 21s (5%), 30s (0%)

@codecov-io
Copy link
Copy Markdown

codecov-io commented Dec 11, 2018

Codecov Report

Merging #436 into master will decrease coverage by 0.21%.
The diff coverage is 56.66%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #436      +/-   ##
============================================
- Coverage      78.5%   78.28%   -0.22%     
- Complexity      646      652       +6     
============================================
  Files            73       73              
  Lines          1847     1870      +23     
  Branches        274      277       +3     
============================================
+ Hits           1450     1464      +14     
- Misses          221      228       +7     
- Partials        176      178       +2

@cryptoman256
Copy link
Copy Markdown

The percentage calc is the wrong denominator. you have 10 expected, 9 actual which is an error of 1. 11% is 1/9. Should be 1/10, 10%.

finalTime += newJunitMap[testCase] ?: 0.0
}

val efficiency = 100 - (expectedTime * 100.0 / finalTime)
Copy link
Copy Markdown

@cryptoman256 cryptoman256 Dec 12, 2018

Choose a reason for hiding this comment

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

if you do it this way, it would be (finalTime * 100 / expectedTime) - 100
Ex. expected 10 sec, actual 9 sec
(9 * 100.0 / 10) - 100 = -10% -----> You were 10% off and it was to the downside
(11 * 100.0 / 10) - 100 = +10% -----> You were 10% off and it was to the upside

Or a more readable error calc imo would be:
(finalTime - expectedTime) / expectedTime * 100
(9-10)/10 * 100 = -10%
(11-10)/10 * 100 = +10%

@inktomi
Copy link
Copy Markdown

inktomi commented Dec 14, 2018

I would recommend printing the time diff instead of a percent diff as well. That seems more actionable as a user - a percentage is not so great as it'd require some math to figure out how long each took compared to the expected time.

Copy link
Copy Markdown
Contributor

@bootstraponline bootstraponline left a comment

Choose a reason for hiding this comment

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

Discussed using human readable time reports:

1h 2m 3s

I agree with the other feedback as well.

@Macarse Macarse force-pushed the macarse/issue428 branch 2 times, most recently from d0dc318 to 8792a40 Compare January 14, 2019 21:51
@Macarse
Copy link
Copy Markdown
Contributor Author

Macarse commented Jan 14, 2019

The output will look something like this:

Smart Flank cache hit: 100% (172 / 172)
Shard times: 46s, 46s, 46s, 46s, 46s, 46s, 47s, 47s, 47s, 47s, 48s, 48s, 48s, 48s, 48s

... 

Actual shard times:
  Shard 0: Expected: 46s, Actual: 46s, Diff: 0s
  Shard 1: Expected: 46s, Actual: 47s, Diff: 1s
  Shard 2: Expected: 46s, Actual: 45s, Diff: -1s
  Shard 3: Expected: 46s, Actual: 46s, Diff: 0s
  Shard 4: Expected: 46s, Actual: 46s, Diff: 0s
  Shard 5: Expected: 46s, Actual: 44s, Diff: -2s
  Shard 6: Expected: 47s, Actual: 48s, Diff: 2s
  Shard 7: Expected: 47s, Actual: 46s, Diff: -1s
  Shard 8: Expected: 47s, Actual: 50s, Diff: 3s
  Shard 9: Expected: 47s, Actual: 47s, Diff: -1s
  Shard 10: Expected: 48s, Actual: 48s, Diff: 1s
  Shard 11: Expected: 48s, Actual: 50s, Diff: 3s
  Shard 12: Expected: 48s, Actual: 48s, Diff: 0s
  Shard 13: Expected: 48s, Actual: 46s, Diff: -2s
  Shard 14: Expected: 48s, Actual: 47s, Diff: -1s

@cryptoman256
Copy link
Copy Markdown

The output will look something like this:

Smart Flank cache hit: 100% (172 / 172)
Shard times: 46s, 46s, 46s, 46s, 46s, 46s, 47s, 47s, 47s, 47s, 48s, 48s, 48s, 48s, 48s

... 

Actual shard times:
  Shard 0: Expected: 46s, Actual: 46s, Diff: 0s
  Shard 1: Expected: 46s, Actual: 47s, Diff: 1s
  Shard 2: Expected: 46s, Actual: 45s, Diff: -1s
  Shard 3: Expected: 46s, Actual: 46s, Diff: 0s
  Shard 4: Expected: 46s, Actual: 46s, Diff: 0s
  Shard 5: Expected: 46s, Actual: 44s, Diff: -2s
  Shard 6: Expected: 47s, Actual: 48s, Diff: 2s
  Shard 7: Expected: 47s, Actual: 46s, Diff: -1s
  Shard 8: Expected: 47s, Actual: 50s, Diff: 3s
  Shard 9: Expected: 47s, Actual: 47s, Diff: -1s
  Shard 10: Expected: 48s, Actual: 48s, Diff: 1s
  Shard 11: Expected: 48s, Actual: 50s, Diff: 3s
  Shard 12: Expected: 48s, Actual: 48s, Diff: 0s
  Shard 13: Expected: 48s, Actual: 46s, Diff: -2s
  Shard 14: Expected: 48s, Actual: 47s, Diff: -1s

I like this, fine with me!

@Macarse Macarse merged commit c2f1052 into master Jan 15, 2019
@bootstraponline bootstraponline deleted the macarse/issue428 branch January 15, 2019 17:50
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.

6 participants