jobs/frontier: add randomized frontier test #153458
Conversation
|
This change is |
msbutler
requested review from
kev-cao and
andyyang890
and removed request for
a team
| } | ||
|
|
||
| func checkSpanFrontierEquality(t *testing.T, expected span.Frontier, actual span.Frontier) { | ||
| for eSp, eTs := range expected.Entries() { |
There was a problem hiding this comment.
This loop technically only checks that expected is a subset of actual. Should we also assert that the number of entries is the same (maybe with .Len())?
There was a problem hiding this comment.
good point. added a len check.
| require.NoError(t, err) | ||
| } | ||
|
|
||
| jobID := jobspb.JobID(3000) |
There was a problem hiding this comment.
nit: if you want to keep the trend of each test in this file generating different job IDs, consider making this 4000
There was a problem hiding this comment.
I don't think that really adds any test coverage. each test uses its own test server.
| newTs := hlc.Timestamp{WallTime: int64(rng.Intn(4))} | ||
|
|
||
| // Only update if new timestamp is greater (frontiers only move forward) | ||
| if initialTimestamps[spanIdx].Less(newTs) { |
There was a problem hiding this comment.
You technically don't need to do this check since Forward will no-op if you're not forwarding to a later time
There was a problem hiding this comment.
ah right! removed that check and it simplifies things slightly.
Epic: none Release note: none
msbutler
force-pushed
the
butler-job-frontier-random
branch
from
68ee697 to
b4e47cf
Compare
| require.NoError(t, err) | ||
| } | ||
|
|
||
| jobID := jobspb.JobID(3000) |
There was a problem hiding this comment.
I don't think that really adds any test coverage. each test uses its own test server.
| newTs := hlc.Timestamp{WallTime: int64(rng.Intn(4))} | ||
|
|
||
| // Only update if new timestamp is greater (frontiers only move forward) | ||
| if initialTimestamps[spanIdx].Less(newTs) { |
There was a problem hiding this comment.
ah right! removed that check and it simplifies things slightly.
| } | ||
|
|
||
| func checkSpanFrontierEquality(t *testing.T, expected span.Frontier, actual span.Frontier) { | ||
| for eSp, eTs := range expected.Entries() { |
There was a problem hiding this comment.
good point. added a len check.
Epic: none
Release note: none