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 test for ExecutionSegment.Scale consistency #1374

Merged
merged 2 commits into from
Mar 30, 2020

Conversation

imiric
Copy link
Contributor

@imiric imiric commented Mar 26, 2020

This is a test from #1339, which did a bunch of things that are not needed anymore, so this is a cut-down version of that.

See #1296

@imiric imiric requested review from mstoykov and na-- March 26, 2020 11:55
lib/execution_segment_test.go Outdated Show resolved Hide resolved
lib/execution_segment_test.go Outdated Show resolved Hide resolved
@codecov-io
Copy link

codecov-io commented Mar 26, 2020

Codecov Report

Merging #1374 into new-schedulers will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@                Coverage Diff                 @@
##           new-schedulers    #1374      +/-   ##
==================================================
- Coverage           76.32%   76.30%   -0.02%     
==================================================
  Files                 161      161              
  Lines               12625    12625              
==================================================
- Hits                 9636     9634       -2     
- Misses               2479     2481       +2     
  Partials              510      510              
Impacted Files Coverage Δ
lib/executor/vu_handle.go 94.28% <0.00%> (-2.86%) ⬇️

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 6507287...0a2bc9c. Read the comment docs.

@imiric imiric requested a review from na-- March 26, 2020 16:31
Copy link
Contributor

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

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

My only nit is not a requirement


const numTests = 10
for i := 0; i < numTests; i++ {
scale := rand.Int31n(99) + 2
Copy link
Contributor

Choose a reason for hiding this comment

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

As I previously rebased these particular tests and used for testing the striping PR I can tell you that having values that are both smaller then the count of the segments and bigger is ... a good idea :D

I think that we can do something like:

  1. get the number of segments(this is the first argument to generateRandomSequence
  2. generate random numbers and run test for scale being smaller, equal and bigger to the count of segments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hhmm not sure what that would look like, but I'll think about it next time I touch these tests, thanks.

@imiric imiric merged commit b14db8e into new-schedulers Mar 30, 2020
@imiric imiric deleted the test/1007-1296-segment-scale branch March 30, 2020 09:02
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.

4 participants