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

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

Merged
merged 1 commit into from
Jan 15, 2019

Conversation

Macarse
Copy link
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

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

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

@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

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
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
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

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