Refactoring integration tests #830

yehor-blox · 2023-01-31T18:01:34Z

refactor integration tests
remove F+1 test scenario

fix require outputs

aaron-blox

some high level comments. was curious about code changes outside of integration test directory, if they are needed/intended to be in this PR

.github/workflows/unit-test.yml

.github/workflows/lint.yml

protocol/v2/ssv/validator/startup.go

protocol/v2/ssv/validator/msgqueue_consumer.go

protocol/v2/qbft/instance/instance.go

integration/qbft/tests/utils.go

network/p2p/test_utils.go

integration/qbft/tests/setup_test.go

aaron-blox

please fix linting errors

Co-authored-by: aaron-blox <123868031+aaron-blox@users.noreply.github.com>

nivBlox · 2023-02-16T09:14:05Z

integration/qbft/tests/scenario_test.go

+
+				duty := createDuty(getKeySet(s.Committee).ValidatorPK.Serialize(), dutyProp.Slot, dutyProp.ValidatorIndex, role)
+				require.NoError(t, s.validators[id].StartDuty(duty))
+			}(id, dutyProp)


why not add to queue? in order to mock real behaviour

@egorka-sereda @aaron-blox this is resolved?

is resolved

nivBlox · 2023-02-16T09:14:32Z

integration/qbft/tests/scenario_test.go

+			// logging stored instance
+			jsonInstance, err := json.Marshal(storedInstance)
+			require.NoError(t, err)
+			fmt.Println(string(jsonInstance))


need to print it?

yeah, for debugging purposes

@egorka-sereda @aaron-blox not sure we need to print the instance.
maybe we can print just the state, or add some debug logging to tests for debug

is resolved

nivBlox · 2023-02-16T09:15:46Z

integration/qbft/tests/scenario_test.go

+		panic(err)
+	}
+
+	store := qbftstorage.New(db, logger, "integration-tests", protocolforks.GenesisForkVersion)


the prefix should be based on the role type and not "integration-tests"
so need to have different instance for each role type

liorrutenberg · 2023-02-19T11:38:12Z

integration/qbft/tests/scenario_test.go

+			// logging stored instance
+			jsonInstance, err := json.Marshal(storedInstance)
+			require.NoError(t, err)
+			fmt.Println(string(jsonInstance))


@egorka-sereda @aaron-blox not sure we need to print the instance.
maybe we can print just the state, or add some debug logging to tests for debug

liorrutenberg · 2023-02-19T11:40:16Z

integration/qbft/tests/scenario_test.go

+}
+
+func getKeySet(committee int) *spectestingutils.TestKeySet {
+	switch committee {


@aaron-blox lets use here just the 3f+1 rule. and max to 13

sometimes we need different number of participants than 4, 7, 10, 13
for example in f+1 decided we want to launch only 3 validators, but we need keysets for them as for 4 validators-committee

liorrutenberg · 2023-02-19T11:41:29Z

integration/qbft/tests/scenario_test.go

+}
+
+func quorum(committee int) int {
+	return (committee*2 + 1) / 3 // committee = 3f+1; quorum = 2f+1 // https://drive.google.com/file/d/1bP_MLq0MM7ZBSR0Ddh7HUPcc42vVUKwz/view?usp=share_link


lets remove google drive links.
If we feel we need it then add readme and resource folder in the integration folder

liorrutenberg · 2023-02-19T12:04:42Z

integration/qbft/tests/regular_test.go

@@ -0,0 +1,111 @@
+package tests


@aaron-blox while this is still WIP, lets remove from the github actions to run integration tests until they are stable.
you can create a regex rule to run it if the branch starts/ends with integration-test

…ration-tests

yehor-blox · 2023-02-20T12:00:52Z

fixed all comments
@lior-blox @aaron-blox
waiting for next review round
Thanks!

integration/qbft/tests/scenario_test.go

aaron-blox

i added a suggestion to change the comment but LGTM otherwise

Co-authored-by: aaron-blox <123868031+aaron-blox@users.noreply.github.com>

aaron-blox · 2023-02-21T07:17:50Z

integration/qbft/tests/round_change_test.go

+			require.Fail(t, "no commit qourum")
+		}
+
+		require.Equal(t, int(spectypes.OperatorID(2)), int(actual.State.ProposeContainer.Msgs[2][0].Signers[0])) //on second round proposer is 2,


you should add checks before doing slice access for everything here
actual.State.ProposeContainer.Msgs[2][0].Signers[0]
Each of these can cause a panic if you are trying to access something that does not exist, which can happen when the test fails

…nto refactoring-integration-tests

nivBlox · 2023-02-22T12:18:11Z

.github/workflows/integration-test.yml

-    branches:
-      - "main"
-      - "stage"
+#  push:


if no need, let's remove

nivBlox · 2023-02-22T12:19:21Z

integration/qbft/tests/regular_test.go

+		require.Equal(t, int(qbft.FirstRound), int(actual.State.Round), "round not matching")
+
+		require.NotNil(t, actual.DecidedMessage, "no decided message")
+		if quorum(committee) > len(actual.DecidedMessage.Signers) {


you can use require.Greater() instead

nivBlox · 2023-02-22T12:19:44Z

integration/qbft/tests/round_change_test.go

+		require.Equal(t, int(qbft.Round(2)), int(actual.State.Round), "round not matching")
+
+		require.NotNil(t, actual.DecidedMessage, "no decided message")
+		if quorum(committee) > len(actual.DecidedMessage.Signers) {


you can use require.Greater() instead

nivBlox · 2023-02-22T12:25:45Z

integration/qbft/tests/scenario_test.go

+		panic(err)
+	}
+
+	store := qbftstorage.New(db, logger, role.String(), protocolforks.GenesisForkVersion)


need to create store for each role.
reference - operator/node.go:91

nivBlox · 2023-02-22T12:26:58Z

integration/qbft/tests/setup_test.go

+}
+
+func GetSharedData(t *testing.T) SharedData { //singleton B-)
+	if sharedData == nil {


let's use require.NotNil

nivBlox · 2023-02-22T12:30:55Z

protocol/v2/qbft/controller/future_msg.go

@@ -17,6 +17,7 @@ func (c *Controller) UponFutureMsg(msg *specqbft.SignedMessage) (*specqbft.Signe
 	}
 	if c.f1SyncTrigger() {
 		c.logger.Debug("triggered f+1 sync",
+			zap.Any("operator-id", c.Share.OperatorID),


it's not a place for this log. please revert. we can open a issue to add this if needed

nivBlox · 2023-02-22T12:31:13Z

protocol/v2/qbft/instance/instance.go

@@ -52,7 +52,7 @@ func NewInstance(
 		config:      config,
 		processMsgF: spectypes.NewThreadSafeF(),
 		metrics:     newMetrics(msgId),
-		logger: logger.With(zap.String("publicKey", hex.EncodeToString(msgId.GetPubKey())), zap.String("role", msgId.GetRoleType().String()),
+		logger: logger.With(zap.Int("operator-id", int(share.OperatorID)), zap.String("publicKey", hex.EncodeToString(msgId.GetPubKey())), zap.String("role", msgId.GetRoleType().String()),


please revert it as it not needed in this scope

nivBlox · 2023-02-22T12:31:21Z

protocol/v2/ssv/validator/msgqueue_consumer.go

@@ -102,7 +102,7 @@ func (v *Validator) ConsumeQueue(msgID spectypes.MessageID, handler MessageHandl

 		// Handle the message.
 		if err := handler(msg); err != nil {
-			v.logMsg(msg, "could not handle message", zap.Any("type", msg.SSVMessage.MsgType), zap.Error(err))
+			v.logMsg(msg, "could not handle message", zap.Int("operator-id", int(v.Share.OperatorID)), zap.Any("type", msg.SSVMessage.MsgType), zap.Error(err))


let's remove

nivBlox · 2023-02-22T12:33:44Z

protocol/v2/ssv/validator/startup.go

@@ -20,6 +20,7 @@ func (v *Validator) Start() error {
 		for role, r := range v.DutyRunners {
 			share := r.GetBaseRunner().Share
 			if share == nil { // TODO: handle missing share?
+				v.logger.Warn("share is missing")


let's add the runner role type to the log

yehor-blox marked this pull request as ready for review February 2, 2023 15:18

yehor-blox force-pushed the refactoring-integration-tests branch from aea4982 to 0c0e0b5 Compare February 2, 2023 15:18

yehor-blox force-pushed the refactoring-integration-tests branch from 50edd30 to 2c4223a Compare February 13, 2023 11:13

yehor-blox marked this pull request as draft February 13, 2023 14:53

yehor-blox force-pushed the refactoring-integration-tests branch 3 times, most recently from eebddfe to e05c34a Compare February 13, 2023 15:03

squashed all commits

74a966e

yehor-blox force-pushed the refactoring-integration-tests branch from e05c34a to 74a966e Compare February 13, 2023 15:04

cut out all what related to f+1 scenario (to initial instances)

af52d30

fix require outputs

yehor-blox marked this pull request as ready for review February 13, 2023 17:13

yehor-blox changed the title ~~DRAFT: Refactoring integration tests~~ Refactoring integration tests Feb 13, 2023

aaron-blox reviewed Feb 14, 2023

View reviewed changes

git issues

2a69b2e

aaron-blox reviewed Feb 16, 2023

View reviewed changes

integration/qbft/tests/setup_test.go Outdated Show resolved Hide resolved

aaron-blox reviewed Feb 16, 2023

View reviewed changes

yehor-blox and others added 2 commits February 16, 2023 10:47

linter issues

3ed3354

Update integration/qbft/tests/setup_test.go

7e09aad

Co-authored-by: aaron-blox <123868031+aaron-blox@users.noreply.github.com>

yehor-blox requested a review from aaron-blox February 16, 2023 08:48

nivBlox suggested changes Feb 16, 2023

View reviewed changes

git issues

4bdf5cb

yehor-blox requested review from nivBlox and removed request for aaron-blox February 16, 2023 14:01

liorrutenberg requested changes Feb 19, 2023

View reviewed changes

yehor-blox added 2 commits February 20, 2023 11:44

git issues

fe2c3ef

Merge branch 'stage' of github.com:bloxapp/ssv into refactoring-integ…

f1a0ee8

…ration-tests

aaron-blox reviewed Feb 20, 2023

View reviewed changes

integration/qbft/tests/scenario_test.go Outdated Show resolved Hide resolved

aaron-blox self-requested a review February 20, 2023 12:11

aaron-blox approved these changes Feb 20, 2023

View reviewed changes

aaron-blox requested a review from liorrutenberg February 20, 2023 12:17

Update integration/qbft/tests/scenario_test.go

cec1b02

Co-authored-by: aaron-blox <123868031+aaron-blox@users.noreply.github.com>

aaron-blox reviewed Feb 21, 2023

View reviewed changes

yehor-blox added 4 commits February 21, 2023 11:14

git issues

29cccbd

Merge remote-tracking branch 'origin/refactoring-integration-tests' i…

484cbb6

…nto refactoring-integration-tests

git issues

71928b2

git issues

b28c233

aaron-blox self-requested a review February 21, 2023 10:00

aaron-blox approved these changes Feb 21, 2023

View reviewed changes

aaron-blox mentioned this pull request Feb 22, 2023

IO1-2633: Fix race conditions in integration tests #850

Merged

nivBlox suggested changes Feb 22, 2023

View reviewed changes

git issues

bdb5608

nivBlox approved these changes Feb 22, 2023

View reviewed changes

liorrutenberg approved these changes Feb 22, 2023

View reviewed changes

hot fix

956c889

liorrutenberg approved these changes Feb 22, 2023

View reviewed changes

liorrutenberg merged commit ddcae3a into ssvlabs:stage Feb 22, 2023

yehor-blox mentioned this pull request Feb 26, 2023

RTM: CreateAndStartLocalNet func optimization #823

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactoring integration tests #830

Refactoring integration tests #830

yehor-blox commented Jan 31, 2023 •

edited

Loading

aaron-blox left a comment •

edited

Loading

aaron-blox left a comment

nivBlox Feb 16, 2023

liorrutenberg Feb 19, 2023

aaron-blox Feb 20, 2023

nivBlox Feb 16, 2023

yehor-blox Feb 16, 2023

liorrutenberg Feb 19, 2023

aaron-blox Feb 20, 2023

nivBlox Feb 16, 2023

aaron-blox Feb 20, 2023

liorrutenberg Feb 19, 2023

liorrutenberg Feb 19, 2023

yehor-blox Feb 20, 2023

liorrutenberg Feb 19, 2023

aaron-blox Feb 20, 2023

liorrutenberg Feb 19, 2023

aaron-blox Feb 20, 2023

yehor-blox commented Feb 20, 2023

aaron-blox left a comment

aaron-blox Feb 21, 2023 •

edited

Loading

nivBlox Feb 22, 2023

nivBlox Feb 22, 2023

nivBlox Feb 22, 2023

nivBlox Feb 22, 2023

nivBlox Feb 22, 2023

nivBlox Feb 22, 2023

nivBlox Feb 22, 2023

nivBlox Feb 22, 2023

nivBlox Feb 22, 2023

Refactoring integration tests #830

Refactoring integration tests #830

Conversation

yehor-blox commented Jan 31, 2023 • edited Loading

aaron-blox left a comment • edited Loading

Choose a reason for hiding this comment

aaron-blox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yehor-blox commented Feb 20, 2023

aaron-blox left a comment

Choose a reason for hiding this comment

aaron-blox Feb 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yehor-blox commented Jan 31, 2023 •

edited

Loading

aaron-blox left a comment •

edited

Loading

aaron-blox Feb 21, 2023 •

edited

Loading