writeToSequential: improve tests for write errors#499
Conversation
There was a problem hiding this comment.
If we rebreak the code, will the test actually fail? I cloned the code locally, and checked, and it seems like it does not.
Monkeying around with things, I think we need to use MaxPacketChecked(3), seed text of "12345", change the check for len(b) == 2 to len(b) > 3, and the test is written != 4.
Reasons:
- With seed text
"hello world"we are not reachingEOFof the input before the read error - With seed text
"1234"andMaxPacketChecked(2)the original code does not fail - With seed text
"12345"andMaxPacketChecked(3)the second call is made withlen(b) == 2notlen(b) == 3; so we needlen(b) > 3. - With the new
MaxPacketChecked(3)we now do one 3-byte write, and one 1-byte write. This also allows us to distinguish from bad-behavior full-write 3+2, from good-behavior 3+1.
🙃 This corner case is actually pretty hard to reproduce intentionally with minimal code.
|
If now you set the seed text to |
puellanivis
left a comment
There was a problem hiding this comment.
these are just nitpicks.
| expected int | ||
| written int | ||
| writtenReturn int | ||
| writeCounter int |
There was a problem hiding this comment.
[nit] we don’t really need the write of the writeCounter. This is a writer after all.
| if written != 4 { | ||
| t.Errorf("sftpFile.Write() = %d, but expected 4", written) | ||
| } | ||
| assert.Equal(t, 2, w.writeCounter) |
There was a problem hiding this comment.
[nit] This test is relying on the default typing of untyped integers assigned to interface{} to be int, and thus not as stable as it could be.
puellanivis
left a comment
There was a problem hiding this comment.
Go ahead and merge as you will with or without addressing the nits.
If you have other comments I will be glad to fix the code, as always thanks! |
| w.counter++ | ||
| if w.counter == 1 { | ||
| if len(b) != 3 { | ||
| return 0, errors.New("this writer require maxPacket = 3, please set MaxPacketChecked(3)") |
puellanivis
left a comment
There was a problem hiding this comment.
You have exhausted all my criticism.
I tryed to improve test cases for write errors as requested in #494
Of course I kept the author for the original commits