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

transport tests: add deadline tests #2286

Merged
merged 14 commits into from Aug 19, 2023
Merged

transport tests: add deadline tests #2286

merged 14 commits into from Aug 19, 2023

Conversation

MarcoPolo
Copy link
Contributor

@MarcoPolo MarcoPolo commented May 11, 2023 •
edited

Integrations test for deadlines

related to #2194

@marten-seemann
Copy link
Contributor

These are the deadlines I had in mind:

go-libp2p/core/network/mux.go

Lines 59 to 61 in 301a197

SetDeadline(time.Time) error
SetReadDeadline(time.Time) error
SetWriteDeadline(time.Time) error

The NewStream deadline test seems useful. It might be worth fixing the transports that are failing the test.

The Dial deadline test is not really a transport test, but a swarm test. There's no point testing it with different transports, is there? Unless you want to assert that the underlying transport dial is actually canceled in flight, which is much harder to test.

@p-shahi p-shahi mentioned this pull request May 15, 2023
Closed
11 tasks
@MarcoPolo
Copy link
Contributor Author

mplex does not seem to pass these tests in CI. I'm going to skip them for now

marten-seemann
marten-seemann reviewed Jun 5, 2023
View reviewed changes
p2p/test/transport/deadline_test.go Outdated Show resolved Hide resolved
p2p/test/transport/deadline_test.go Outdated Show resolved Hide resolved
p2p/test/transport/deadline_test.go Outdated Show resolved Hide resolved
p2p/test/transport/deadline_test.go Outdated Show resolved Hide resolved
p2p/test/transport/deadline_test.go Outdated Show resolved Hide resolved
@MarcoPolo MarcoPolo mentioned this pull request Jun 7, 2023
Merged
@MarcoPolo
Copy link
Contributor Author

friendly ping @marten-seemann. This is the last PR for the transport integration test for #2194

marten-seemann
marten-seemann reviewed Aug 10, 2023
View reviewed changes
Copy link
Contributor

@marten-seemann marten-seemann left a comment

Choose a reason for hiding this comment

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

Can you rebase on top of master, there might have been changes that could cause these tests to fail / pass now.

p2p/test/transport/deadline_test.go Outdated Show resolved Hide resolved
p2p/test/transport/deadline_test.go Outdated Show resolved Hide resolved
p2p/test/transport/deadline_test.go Outdated Show resolved Hide resolved
p2p/test/transport/deadline_test.go Outdated Show resolved Hide resolved
p2p/test/transport/deadline_test.go
// Set a deadline
s.SetWriteDeadline(time.Now().Add(10 * time.Millisecond))
start := time.Now()
_, err = s.Write(sendBuf)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why would this run into the deadline? We could just send all the data, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The sendBuf is 1 MiB. Unless we can send 1 MiB in 10ms at the start this would hit the deadline. I could increase the sendbuf to 10 MiB instead to make it even less likely.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

bumped to 10MiB

p2p/test/transport/deadline_test.go
})

// Like the above, but with SetDeadline
t.Run("SetDeadline", func(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if we need this test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why not? Is the thought that this is simply calling Set{Read,Write}Deadline? I don't think that is enforced and a transport could forget to implement this.

@marten-seemann marten-seemann changed the title Transport Integration tests: OpenStream and DialPeer deadlines transport tests: OpenStream and DialPeer deadlines Aug 19, 2023
@marten-seemann marten-seemann changed the title transport tests: OpenStream and DialPeer deadlines transport tests: add deadline tests Aug 19, 2023
@marten-seemann marten-seemann merged commit ddc7238 into master Aug 19, 2023
21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marten-seemann marten-seemann marten-seemann approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@MarcoPolo @marten-seemann