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

loadtest: prepare for multiple consecutive runs #550

Merged
merged 5 commits into from
Oct 7, 2023

Conversation

guggero
Copy link
Member

@guggero guggero commented Oct 5, 2023

Fixes #539.

This PR allows the load test to be run against both btcd and bitcoind and also makes it ready for being run multiple times in a row against the same node.

cc @calvinrzachman

@guggero guggero requested review from ffranr and jharveyb October 5, 2023 11:53
@@ -20,7 +22,9 @@ import (
)

var (
zeroHash chainhash.Hash
zeroHash chainhash.Hash
regtestMiningAddr = "n1VgRjYDzJT2TV72PnungWgWu18SWorXZS"
Copy link
Member

Choose a reason for hiding this comment

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

Is seed generation by the nodes deterministic?

Do we need to also move to simnet, so bitcoind won't wipe the chain on start up?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is just to send to bitcoind to specify where to mine the the coins to. We don't really ever use that address for anything else.

No, it's btcd that drops the chain on startup, not bitcoind. So we can switch batch to bitcoind and regtest.

t, alice, groupBalance, nil, collectGroupKey,
)
require.NoError(t, err)
AssertUniverseRoot(t, alice, groupBalance, nil, collectGroupKey)
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't this just fail the test either way?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, but before it was a mixture of require and returning an error. And now we just do t.Fatalf() directly instead of returning the error and then doing require.NoError() outside.

Copy link
Contributor

@calvinrzachman calvinrzachman left a comment

Choose a reason for hiding this comment

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

Thanks for helping me out here! Just had a similar question to Laolu re: the hardcoded mining address. I will give the changes a try in staging

@@ -194,7 +196,18 @@ func mintBatchStressTest(t *testing.T, ctx context.Context,
},
},
)
require.NoError(t, err)
if err != nil {
require.ErrorContains(t, err, "universe server already added")
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: not a big deal either way, but could make use of the defined error type here. Might be a bit more robust to changes in the error string.

      if err != nil {
               // Do not fail the test if we already have added the universe server!
               require.ErrorIsf(t, err, universe.ErrDuplicateUniverse,
                       "unexpected error while adding remote universe server: %v", err)
       }

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point! Though we cannot use ErrorIs because the error is being sent over RPC and cannot directly be unwrapped/matched. But we can still use the error constant in ErrorContains, which I'll fix.

Copy link
Member

@Roasbeef Roasbeef left a comment

Choose a reason for hiding this comment

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

LGTM 🪁

@guggero guggero force-pushed the loadtest-multiple-times branch from 4a10dd8 to 0afdb54 Compare October 6, 2023 07:46
Copy link
Contributor

@ffranr ffranr left a comment

Choose a reason for hiding this comment

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

Looks ok to me, just two nits.

itest/assertions.go Outdated Show resolved Hide resolved
itest/loadtest/mint_batch_test.go Outdated Show resolved Hide resolved
@guggero guggero force-pushed the loadtest-multiple-times branch from 0afdb54 to e5d4a15 Compare October 6, 2023 14:45
@guggero guggero requested a review from ffranr October 6, 2023 14:46
@Roasbeef Roasbeef added this pull request to the merge queue Oct 6, 2023
Merged via the queue into main with commit b8af77d Oct 7, 2023
14 checks passed
@guggero guggero deleted the loadtest-multiple-times branch October 7, 2023 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

[feature]: allow the loadtest binary to run repeatedly against persistent tapd instances
4 participants