-
Notifications
You must be signed in to change notification settings - Fork 16
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
quic: readfrom/writeto saver, updated tests #1176
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect one more change is needed? (Thank you for doing this so quickly and for adding the additional test checking what happens with a wrong IP!)
netx/archival/archival.go
Outdated
@@ -492,6 +492,7 @@ func NewNetworkEventsList(begin time.Time, events []trace.Event) []NetworkEvent | |||
} | |||
if ev.Name == errorx.ReadOperation { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think here you should handle also ReadFromOperation
?
netx/archival/archival.go
Outdated
@@ -501,6 +502,7 @@ func NewNetworkEventsList(begin time.Time, events []trace.Event) []NetworkEvent | |||
} | |||
if ev.Name == errorx.WriteOperation { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think here you should also handle WriteToOperation
?
netx/errorx/errorx.go
Outdated
@@ -74,6 +74,12 @@ const ( | |||
// WriteOperation is when we write to a socket | |||
WriteOperation = "write" | |||
|
|||
// ReadFromOperation is when we read from an UDP socket | |||
ReadFromOperation = "readfrom" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use _
to separate other operations, so please use read_from
netx/errorx/errorx.go
Outdated
ReadFromOperation = "readfrom" | ||
|
||
// WriteToOperation is when we write to an UDP socket | ||
WriteToOperation = "writeto" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be write_to
netx/quicdialer/system_test.go
Outdated
@@ -11,6 +11,44 @@ import ( | |||
"github.com/ooni/probe-engine/netx/trace" | |||
) | |||
|
|||
func TestSystemDialerSuccess(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test seems redundant? Isn't this a subset of TestSystemDialerSuccessWithReadWrite
?
netx/quicdialer/system_test.go
Outdated
if err.Error() != "quicdialer: invalid IP representation" { | ||
t.Fatal("expected another error here") | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! 🥳🦜
No description provided.