Add Client.Sync method #386
Conversation
client_integration_test.go
Outdated
| // XXX We can't distinguish an actual fsync not supported | ||
| // error from a typo in the extension name here. | ||
| t.Logf("Sync: %v", err) | ||
| assert.Equal(t, ErrSSHFxOpUnsupported, err.FxCode()) |
There was a problem hiding this comment.
So, there are two paths on this where this test can pass: either err == nil or err.(*StatusError).FxCode() == ErrSSHFxOpUnsupported. I’m kind of hesitant on having more than one way a test can pass. A test should be well-constructed enough to ensure that there is only one valid behavior, and thus only one codepath that should pass the test.
Is this running against our own server implementation, which is why we account that it might return “unsupported”? Then maybe we should write support for it, or at least a stub of a support somehow, just so that it is not met immediately with unsupported.
There was a problem hiding this comment.
Yes, this is primarily so I don't have to write the server part. I only use the client. We could check whether the test is running against a real server and be stricter in that case; OpenSSH has had this extension since 2013.
There was a problem hiding this comment.
I would highly recommend against making the tests smarter than they need to be, like, checking to see if the server supports the feature or not, before running the test. You end up in a situation where the test is no longer clear, and “is this passing because it actually behaves as we expect it to, or because the server unexpected does not support the feature.”
I would recommend instead putting it in its own TestClientSync that will expect only the SSHFxOpUnsupported case, and explicitly document with comments that this test will need to be reworked once the internal server actually supports the function. (Which the implementer should come across, because the test should actually fail at that point.)
There was a problem hiding this comment.
I've replaced this by two integration tests, including one for an actual server. I do like to check whether the method actually works with OpenSSH :)
There was a problem hiding this comment.
Great solution! Thanks for sticking through, sorry about the delays.
greatroar
force-pushed
the
fsync
branch
4 times, most recently
from
e5bb489
to
5d9557c
Compare
This uses the fsync@openssh.com extension: https://github.com/openssh/openssh-portable/blob/master/PROTOCOL, §3.6.
|
|
||
| // Since Server does not support the fsync extension, we can only | ||
| // check that we get the right error. | ||
| require.NotNil(t, err) |
There was a problem hiding this comment.
Oof, I didn’t check the usage of require and assert as closely as I should have. 🤦♀️
This uses the
fsync@openssh.comextension: https://github.com/openssh/openssh-portable/blob/master/PROTOCOL, §3.6.