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.

Sign up for GitHub

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

Jump to bottom

core, trie, rpc: speed up tests #28461

Merged
merged 7 commits into from
Nov 21, 2023
Merged

core, trie, rpc: speed up tests #28461

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
b051500
rpc: make subscription test faster
MariusVanDerWijden Nov 3, 2023
c413663
trie: cache trie nodes for faster sanity check
MariusVanDerWijden Nov 3, 2023
cb349b0
core/forkid: speed up validation test
MariusVanDerWijden Nov 3, 2023
be78565
core/state: improve snapshot test run
MariusVanDerWijden Nov 3, 2023
62a3291
accounts/keystore: improve keyfile test
MariusVanDerWijden Nov 3, 2023
08be7f1
trie: remove resolver
MariusVanDerWijden Nov 15, 2023
65bebf3
trie: only check ~5% of all trie nodes
MariusVanDerWijden Nov 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions accounts/keystore/account_cache_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ func waitWatcherStart(ks *KeyStore) bool {

func waitForAccounts(wantAccounts []accounts.Account, ks *KeyStore) error {
var list []accounts.Account
for t0 := time.Now(); time.Since(t0) < 5*time.Second; time.Sleep(200 * time.Millisecond) {
for t0 := time.Now(); time.Since(t0) < 5*time.Second; time.Sleep(100 * time.Millisecond) {
list = ks.Accounts()
if reflect.DeepEqual(list, wantAccounts) {
// ks should have also received change notifications
Expand Down Expand Up @@ -350,7 +350,7 @@ func TestUpdatedKeyfileContents(t *testing.T) {
return
}
// needed so that modTime of `file` is different to its current value after forceCopyFile
time.Sleep(time.Second)
os.Chtimes(file, time.Now().Add(-time.Second), time.Now().Add(-time.Second))
holiman marked this conversation as resolved.
Show resolved Hide resolved

// Now replace file contents
if err := forceCopyFile(file, cachetestAccounts[1].URL.Path); err != nil {
Expand All @@ -366,7 +366,7 @@ func TestUpdatedKeyfileContents(t *testing.T) {
}

// needed so that modTime of `file` is different to its current value after forceCopyFile
time.Sleep(time.Second)
os.Chtimes(file, time.Now().Add(-time.Second), time.Now().Add(-time.Second))

// Now replace file contents again
if err := forceCopyFile(file, cachetestAccounts[2].URL.Path); err != nil {
Expand All @@ -382,7 +382,7 @@ func TestUpdatedKeyfileContents(t *testing.T) {
}

// needed so that modTime of `file` is different to its current value after os.WriteFile
time.Sleep(time.Second)
os.Chtimes(file, time.Now().Add(-time.Second), time.Now().Add(-time.Second))
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks a bit odd to me that you are (over and over) setting mtime/atime to time -1. If we want them to change shouldn't you set them to e.g. time-5, time-4, time-3, ... etc?

Copy link
Member Author

@MariusVanDerWijden MariusVanDerWijden Nov 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think that forceCopyFile will always update the timestamp to the current time, thats why we can just set the time to -1 sec


// Now replace file contents with crap
if err := os.WriteFile(file, []byte("foo"), 0600); err != nil {
Expand Down
3 changes: 2 additions & 1 deletion core/forkid/forkid_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -366,8 +366,9 @@ func TestValidation(t *testing.T) {
// TODO(karalabe): Enable this when Cancun is specced
//{params.MainnetChainConfig, 20999999, 1677999999, ID{Hash: checksumToBytes(0x71147644), Next: 1678000000}, ErrLocalIncompatibleOrStale},
}
genesis := core.DefaultGenesisBlock().ToBlock()
for i, tt := range tests {
filter := newFilter(tt.config, core.DefaultGenesisBlock().ToBlock(), func() (uint64, uint64) { return tt.head, tt.time })
filter := newFilter(tt.config, genesis, func() (uint64, uint64) { return tt.head, tt.time })
if err := filter(tt.id); err != tt.err {
t.Errorf("test %d: validation error mismatch: have %v, want %v", i, err, tt.err)
}
Expand Down
8 changes: 3 additions & 5 deletions core/state/statedb_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -426,23 +426,21 @@ func (test *snapshotTest) run() bool {
state, _ = New(types.EmptyRootHash, NewDatabase(rawdb.NewMemoryDatabase()), nil)
snapshotRevs = make([]int, len(test.snapshots))
sindex = 0
checkstates = make([]*StateDB, len(test.snapshots))
)
for i, action := range test.actions {
if len(test.snapshots) > sindex && i == test.snapshots[sindex] {
snapshotRevs[sindex] = state.Snapshot()
checkstates[sindex] = state.Copy()
sindex++
}
action.fn(action, state)
}
// Revert all snapshots in reverse order. Each revert must yield a state
// that is equivalent to fresh state with all actions up the snapshot applied.
for sindex--; sindex >= 0; sindex-- {
checkstate, _ := New(types.EmptyRootHash, state.Database(), nil)
for _, action := range test.actions[:test.snapshots[sindex]] {
action.fn(action, checkstate)
}
state.RevertToSnapshot(snapshotRevs[sindex])
if err := test.checkEqual(state, checkstate); err != nil {
if err := test.checkEqual(state, checkstates[sindex]); err != nil {
test.err = fmt.Errorf("state mismatch after revert to snapshot %d\n%v", sindex, err)
return false
}
Expand Down
2 changes: 1 addition & 1 deletion rpc/client_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -595,7 +595,7 @@ func TestClientSubscriptionChannelClose(t *testing.T) {

for i := 0; i < 100; i++ {
ch := make(chan int, 100)
sub, err := client.Subscribe(context.Background(), "nftest", ch, "someSubscription", maxClientSubscriptionBuffer-1, 1)
sub, err := client.Subscribe(context.Background(), "nftest", ch, "someSubscription", 100, 1)
Copy link
Contributor

Choose a reason for hiding this comment

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

This change makes a huge difference in execution speed, however, the semantics of this test is pretty complicated. Would be good if @fjl checks that the test semantics are still maintained.

Copy link
Member Author

@MariusVanDerWijden MariusVanDerWijden Nov 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

As far as I understood the test it just opens a lot of subscriptions and subscribes to a lot of messages, then shuts down the subscriptions in order to catch a racy error

Copy link
Contributor

Choose a reason for hiding this comment

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

@fjl pls ack if this LGTY?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry for the slow response. It's OK to change this.

if err != nil {
t.Fatal(err)
}
Expand Down
5 changes: 5 additions & 0 deletions trie/sync_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ package trie
import (
"bytes"
"fmt"
"math/rand"
"testing"

"github.com/ethereum/go-ethereum/common"
Expand Down Expand Up @@ -587,6 +588,10 @@ func testIncompleteSync(t *testing.T, scheme string) {
}
// Sanity check that removing any node from the database is detected
for i, path := range addedKeys {
if rand.Int31n(100) > 5 {
// Only check 5 percent of added keys as a sanity check
continue
}
owner, inner := ResolvePath([]byte(path))
nodeHash := addedHashes[i]
value := rawdb.ReadTrieNode(diskdb, owner, inner, nodeHash, scheme)
Expand Down