Add random scaling tests, original #997 scaling algorithm #1339

imiric · 2020-02-28T13:02:06Z

This adds the original scaling algorithm described in #997 as a separate ExecutionSegment.ScaleRemainder function, and adds a couple of random scaling tests, one of which is disabled until the implementation is fixed.

It also updates stretchr/testify in order to use the new Greater* assertions.

This effort is done to determine whether the previous algorithm behaves better when scaling segments, in order to fix #1296. Result: no, it doesn't, but we decided to leave it in for future reference, and will probably delete it later.

lib/execution_segment.go

It has some new handy assertions like Greater(), and the previous version was 2 years old.

This frequently fails on the current implementation.

…s alternative See #1296 (comment)

… fixed See #1296 (comment)

codecov-io · 2020-03-04T11:00:37Z

Codecov Report

Merging #1339 into new-schedulers will decrease coverage by 0.08%.
The diff coverage is 0%.

@@                Coverage Diff                 @@
##           new-schedulers    #1339      +/-   ##
==================================================
- Coverage           76.78%   76.69%   -0.09%     
==================================================
  Files                 160      160              
  Lines               12511    12523      +12     
==================================================
- Hits                 9606     9604       -2     
- Misses               2396     2410      +14     
  Partials              509      509

Impacted Files	Coverage Δ
lib/execution_segment.go	`81.11% <0%> (-5.8%)`	⬇️
lib/helpers.go	`95.83% <0%> (-4.17%)`	⬇️

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 9cb4ed0...2128816. Read the comment docs.

mstoykov

I really don't like the upgrade of the require lib in this PR just to get a small helper function which can be emulated with require.True.
I also happened to copy paste your tests in my #1323 and intended on doing all of the refactorings I propose here , so if we agree on those maybe just drop the testing and benchmarks?
Or maybe wait until we are done with the other PR and then we will add this on top as I don't think we actually will use it anywhere ...

mstoykov · 2020-03-06T08:55:01Z

lib/execution_segment_test.go

+		var i, lastResult int64
+		for i = 1; i < 1000; i++ {
+			result := es.Scale(i)
+			require.GreaterOrEqual(t, result, lastResult)


I don't think upgrading the lib for this is worth it ... especially in this commit

Well, the previous version is 2 years old at this point, and while I agree that we could use require.True for these cases, it makes sense to use a native API if it's already implemented in a version we should upgrade to anyway. :)

especially in this commit

In this PR, you mean? I needed it here, that's why, though let me know if you'd prefer a separate PR for it.

Why are you against upgrading stretchr/testify?

@na--: thoughts?

I don't really mind, especially considering that it's only used in our tests. I think we upgrade dependencies far to rarely anyway...

mstoykov · 2020-03-06T09:09:30Z

lib/execution_segment_test.go

+}
+
+func TestMain(m *testing.M) {
+	rand.Seed(time.Now().UnixNano())


I do prefer if we at least log it somehow so we can at least see it in case it produces an error but just in general it doesn't
I would also really prefer if we actually don't use TestMain, but instead each test has it's own source as I've done in https://github.com/loadimpact/k6/blob/af4321099e8f65f9279e92c8ec46a6f7d3c1cc29/lib/execution_segment_test.go#L493-L494 maybe we can make it use time.Now() for tests and keep it using a constant for the benchmarks?

I do prefer if we at least log it somehow so we can at least see it in case it produces an error but just in general it doesn't

You mean log the chosen seed? Sure, I'll add it.

I would also really prefer if we actually don't use TestMain, but instead each test has it's own source as I've done in

What would be the reason for that? In this case neither test "cares" about its random source, it just needs it to use a different seed on each run, so I don't see what we would gain by initializing a new source separately. After all, tests that need a constant seed can initialize their own and not use the global source, like your example.

maybe we can make it use time.Now() for tests and keep it using a constant for the benchmarks?

The benchmark here doesn't depend on rand, but yeah, benchmarks that do should use a constant seed.

imiric · 2020-03-06T15:02:58Z

I also happened to copy paste your tests in my #1323 and intended on doing all of the refactorings I propose here , so if we agree on those maybe just drop the testing and benchmarks?
Or maybe wait until we are done with the other PR and then we will add this on top as I don't think we actually will use it anywhere ...

Yeah, I guess there's not much value in merging this then. We can close the PR and leave the branch for reference if you want. Let me know.

na-- · 2020-03-26T10:00:36Z

I think merging TestExecutionSegmentScaleConsistency() is very much worth it, so this PR still makes sense to me. We can just delete TestExecutionSegmentScaleNoWobble(), since it won't be used in any way anymore, but the rest still seems valuable to have.

imiric · 2020-03-26T12:00:12Z

Closing this since most of it is irrelevant now, except for TestExecutionSegmentScaleConsistency, which is now in #1374.

imiric requested review from mstoykov and na-- February 28, 2020 13:02

imiric commented Feb 28, 2020

View reviewed changes

lib/execution_segment.go Outdated Show resolved Hide resolved

imiric force-pushed the test/1296-segment-scale branch 3 times, most recently from becf91a to 5ec4714 Compare February 28, 2020 14:40

mstoykov reviewed Feb 28, 2020

View reviewed changes

lib/execution_segment.go Outdated Show resolved Hide resolved

Ivan Mirić added 5 commits March 2, 2020 11:56

Implement first scaling proposal from #997

0cc2a52

Add test for ExecutionSegment.Scale consistency

64d81a6

Upgrade stretchr/testify to latest version

d34c62a

It has some new handy assertions like Greater(), and the previous version was 2 years old.

Add segment scale up wobble test

f3cfce0

This frequently fails on the current implementation.

Add ExecutionSegment.Scale benchmark

c54a7bc

imiric force-pushed the test/1296-segment-scale branch from 5ec4714 to c54a7bc Compare March 2, 2020 11:12

imiric mentioned this pull request Mar 2, 2020

variable-looping-vus bugs #1296

Closed

Ivan Mirić added 3 commits March 4, 2020 11:46

Use big.Int for flooring operation instead of converting to float

11bccb0

Bring back current ExecutionSegment.Scale implementation, leave old a…

5646a87

…s alternative See #1296 (comment)

Disable TestExecutionSegmentScaleNoWobble until the implementation is…

2128816

… fixed See #1296 (comment)

imiric force-pushed the test/1296-segment-scale branch from 777884d to 2128816 Compare March 4, 2020 10:47

imiric changed the title ~~WIP Implement original #997 scaling algorithm~~ Add random scaling tests, original #997 scaling algorithm Mar 5, 2020

imiric marked this pull request as ready for review March 5, 2020 17:17

imiric requested a review from mstoykov March 5, 2020 17:18

mstoykov reviewed Mar 6, 2020

View reviewed changes

imiric mentioned this pull request Mar 26, 2020

Add test for ExecutionSegment.Scale consistency #1374

Merged

imiric closed this Mar 26, 2020

na-- mentioned this pull request Mar 31, 2020

Implement GetStripedOffsets #1323

Merged

mstoykov deleted the test/1296-segment-scale branch July 7, 2020 08:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add random scaling tests, original #997 scaling algorithm #1339

Add random scaling tests, original #997 scaling algorithm #1339

imiric commented Feb 28, 2020 •

edited

Loading

codecov-io commented Mar 4, 2020 •

edited

Loading

mstoykov left a comment

mstoykov Mar 6, 2020

imiric Mar 6, 2020

na-- Mar 26, 2020

mstoykov Mar 6, 2020

imiric Mar 6, 2020

imiric commented Mar 6, 2020

na-- commented Mar 26, 2020

imiric commented Mar 26, 2020

Add random scaling tests, original #997 scaling algorithm #1339

Add random scaling tests, original #997 scaling algorithm #1339

Conversation

imiric commented Feb 28, 2020 • edited Loading

codecov-io commented Mar 4, 2020 • edited Loading

Codecov Report

mstoykov left a comment

Choose a reason for hiding this comment

mstoykov Mar 6, 2020

Choose a reason for hiding this comment

imiric Mar 6, 2020

Choose a reason for hiding this comment

na-- Mar 26, 2020

Choose a reason for hiding this comment

mstoykov Mar 6, 2020

Choose a reason for hiding this comment

imiric Mar 6, 2020

Choose a reason for hiding this comment

imiric commented Mar 6, 2020

na-- commented Mar 26, 2020

imiric commented Mar 26, 2020

imiric commented Feb 28, 2020 •

edited

Loading

codecov-io commented Mar 4, 2020 •

edited

Loading