tests: Fix devmode test #5334
tests: Fix devmode test #5334
Conversation
There was a problem hiding this comment.
Looks promising, but after around 10 minutes hunting for the log that shows the that devmode_test.go ran I couldn't find it. Would you mind providing a link?
(I started my search with the link you previously provided: https://app.circleci.com/pipelines/github/algorand/go-algorand/13978/workflows/ba87d853-cec2-4473-86f4-c7fdd65caa66)
There was a problem hiding this comment.
Looking good - I think the test run is reported on this line: https://app.circleci.com/pipelines/github/algorand/go-algorand/13978/workflows/ba87d853-cec2-4473-86f4-c7fdd65caa66/jobs/227972/parallel-runs/2?filterBy=ALL&invite=true#step-114-453
I can't really tell if this test will be flaky or not based on one run, but it looks okay here (executed within 15.78 seconds, whereas the test will fail after ~19 seconds).
Also I'm not sure if waiting 8 seconds for 2 transaction confirmations is enough to differentiate dev mode from a normal network (or whether this will be future proof if block times get faster)?
| // Without transactions there should be no rounds even after a normal confirmation time. | ||
| time.Sleep(10 * time.Second) |
There was a problem hiding this comment.
What do you think about removing the second phase of this test?
There was a problem hiding this comment.
Seems fine to me.
| require.Equal(t, round-1, uint64(txn.Txn.Txn.FirstValid)) | ||
| newBlk, err := fixture.AlgodClient.Block(round) | ||
| require.NoError(t, err) | ||
| newBlkSeconds, _ := math.Modf(newBlk.Block["ts"].(float64)) |
There was a problem hiding this comment.
Is there a reason to prefer math.Modf(...) over int(...) here?
There was a problem hiding this comment.
On my local machine, removing math.Modf seems to do the same thing and passes the checks:
| newBlkSeconds, _ := math.Modf(newBlk.Block["ts"].(float64)) | |
| newBlkSeconds, _ := newBlk.Block["ts"].(float64) |
There was a problem hiding this comment.
None that I know of.
| newBlk, err := fixture.AlgodClient.Block(round) | ||
| require.NoError(t, err) | ||
| newBlkSeconds, _ := math.Modf(newBlk.Block["ts"].(float64)) | ||
| require.GreaterOrEqual(t, time.Unix(int64(newBlkSeconds), 0), startTime.Add(1_000_000*time.Second)) |
There was a problem hiding this comment.
Is this deterministic now?
| require.GreaterOrEqual(t, time.Unix(int64(newBlkSeconds), 0), startTime.Add(1_000_000*time.Second)) | |
| require.Equal(t, time.Unix(int64(newBlkSeconds), 0), startTime.Add(1_000_000*time.Second)) |
There was a problem hiding this comment.
I think we need to keep it greater than or equal because the offset is just added to the clock time. So two separate txns 1 second apart could have block times 1_000_001 seconds apart if the offset is 1_000_000.
There was a problem hiding this comment.
It should be added to the previous block time, so I'm pretty sure this is deterministic.
@algochoi correct me if I'm wrong.
There was a problem hiding this comment.
That's true @winder . The current test needs to be modified so that the initial seconds or startTime is updated inside the loop. The second iteration will make the delta 2 million seconds so the equal will fail the second time.
| require.Equal(t, round-1, uint64(txn.Txn.Txn.FirstValid)) | ||
| newBlk, err := fixture.AlgodClient.Block(round) | ||
| require.NoError(t, err) | ||
| newBlkSeconds, _ := math.Modf(newBlk.Block["ts"].(float64)) |
There was a problem hiding this comment.
On my local machine, removing math.Modf seems to do the same thing and passes the checks:
| newBlkSeconds, _ := math.Modf(newBlk.Block["ts"].(float64)) | |
| newBlkSeconds, _ := newBlk.Block["ts"].(float64) |
|
Lgtm |
Summary
Re-enables this devmode test.
Closes #3267 (though I've not been able to see the slow/flakiness happen). It's possible that re-enabling this will just cause nightlies to fail periodically, but the nightly run I did has not failed in this way yet.
We should keep an eye on the nightlies and see if this is still causing flakiness.
Test Plan
Ran nightly tests on this to validate that the test passes on them (since it will be skipped during regular per-PR CI runs).
Here is the resulting run: https://app.circleci.com/pipelines/github/algorand/go-algorand/13978/workflows/ba87d853-cec2-4473-86f4-c7fdd65caa66
The one failure in that is from an unrelated issues relating to running the nightly tests on a fork branch.