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

(dot/network) testStreamHandler use a notify channel to remove time.Sleep in tests #2204

Open
EclesioMeloJunior opened this issue Jan 17, 2022 · 4 comments

Comments

@EclesioMeloJunior
Copy link
Member

EclesioMeloJunior commented Jan 17, 2022

Issue summary

  • I noticed that in all tests whose uses testStreamHandler has a time.Sleep(time.Second) to wait for the messages sent to the stream handler to arrive and then call the asserts to check.
  • However, is it possible to testStreamHandler struct has a field messageArrived chan<- struct{} that always a message is arrived sent a message to the test to unblock and exec the tests without time.Sleep(...)

Other information and links

type testStreamHandler struct {
	messageArrived <-chan struct{}
        ...
}

func (s *testStreamHandler) handleMessage(stream libp2pnetwork.Stream, msg Message) error {
	s.messageArrived <- struct{}
	...
}

In the test side

func Test_...(t *testing.T) {
	...
        + handlerMessageArrived := make(chan struct{})
        + handler := newTestStreamHandler(testBlockRequestMessageDecoder, handlerMessageArrived)
        ...
        - time.Sleep(time.Second)
        + <-handlerMessageArrived

        msg, ok := handler.messages[nodeA.host.id()]
	require.True(t, ok)
	require.Equal(t, 1, len(msg))
	require.Equal(t, testBlockReqMessage, msg[0])
}
@EclesioMeloJunior
Copy link
Member Author

@qdm12 @danforbes can this issue be related to tests epic?

@qdm12
Copy link
Contributor

qdm12 commented Jan 17, 2022

Yes you can move it there for now. @danforbes when would you like to convert the testing plan/what we discussed into epics? That would fall under the make our test test suite faster epic I think.

@danforbes
Copy link
Contributor

@qdm12 do you mean #1472?

@qdm12
Copy link
Contributor

qdm12 commented Jan 17, 2022

Exactly! I assigned the issue to the 1472 epic

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants