This is not an efficient way to build mocks in Go. We’re already writing a receiver method here for a type, so we don’t need to wrap a generic function like this.

type errWriter struct{ … } with one receiver method is the same amount of code, and fewer function calls.

You’ve copied code from another test that is irrelevant to the testing here. Please remove this condition as it is irrelevant noise.

content is not used after these two lines. Skip the temporary variable, and just pass it in directly as a function argument.

We do not know for sure here that we’re calling into sftpFile.writeToSequential. Instead, we should just directly actually call the function itself rather than depend on io.Copy to call the expected function.

This magic number is greater than it needs to be, and specified in two places.

We want to ensure that it returns whatever short write value we return, but it should be so clearly under the number of bytes expected to be copied that we can be sure at a glance that it has done a short write, and still returned the short write values. For a value of 10 here, I’m not sure if that’s a full write with error, or supposed to be a short write.

For this, an ideal value is going to be 2. No one is going to think a write of "hello world" is supposed to return 2 bytes written.

Test errors should be as clearly test errors as possible. Prefer errors.New("test error"). Who knows if somewhere in the code, there might be returned an "error writing" error. But nothing should ever return a "test error" error except in test code.

We want this directory name to be as precisely narrow as possible. If we make a TestClientWriteSequential and repeat this TempDir construction, now we have tests that step on each other’s toes.

Add -writeerr to the directory name.

The appropriate scope of assertion here is that it is not io.EOF. We don’t need to test that it’s the exact same error that we returned, (which is why you’re capturing it with want := errors.New(…) because otherwise the error is an opaque pointer and errors.Is(errors.New("foo"), errors.New("foo")) == false)

We just need to know it’s not an io.EOF.

If we want to prioritize returning an error from Write, then we don’t need any of this complicated logic anymore. We can just return err here directly.

m, err := w.Write(b[:n])
written += int64(m)

if err != nil {
  return written, err
}

I was in doubt about this one myself, I do like the early return more too, I'll change it.

Testing just a single write here for written probably isn’t fully reliable. Consider, if the function were implemented as:

m, err := w.Write(b[:n])
if err != nil {
  return m, err
}
written += int64(w)
`̀ `

We might need to use `timeBobWriter` instead, with a very small chunk size?

there’s no need to rename err error. Since we now have an early return, there is no need to access the two err variables at the same time, so shadowing is no longer a problem.

This test is too strict. If we shoot past w.expected then we will never enter this loop.

This alias ends up adding more characters of code than it saves. Sometimes, repetition isn’t a bad thing.

w is only ever access by pointer. So just directly declare it with a pointer: &lastChunkErrWriter (sequential is imprecisely inaccurate in this name; it need not only be used in this test.)

Unfortunately, this test still does not ensure that the same bug mentioned before could still slip through. We’re still only ever making one call to Write() as the first Write still errors out immediately. We have to set chunkSize. We need to set MaxPacketChecked(2) so that we’re writing in say 2-byte chunks. Then we want the first to succeed, then the second return early (1), so we end up with written == 3.

In the face of all of this, I think special casing all of the functionality of the writer is appropriate. First run returns len(b), nil the second returns 1, errors.New("test error"). Then we also test to ensure that the function was called only twice.

os.File.Write: … Write returns a non-nil error when n != len(b).

If we already passed the NoError above, then we can rely upon the contract, that n can be known to be len(content).

Speaking of which, let’s use WriteString so we can have a const for content. But also since as noted below, we don’t need to test for n != len(content) anyways, we don’t need to use content more than once either, so just use the string directly.

We know logically that we could not have reached this code unless err == nil. So, there’s no point in renaming err here, as the previous value is already irrelevant, so the name can be reused.

Likewise, there will now be no other gotXY, so, we can shorten that variable name to just that: got.

The API is confusing on this matter:
func ErrorIs(t TestingT, err error, target error, msgAndArgs ...interface{}),
but:
func Equal(t TestingT, expected interface{}, actual interface{}, msgAndArgs ...interface{})

This is one of the central reasons why I tell people to avoid using require and assert. Getting the arguments in the wrong order is way too easy to do, and without checking the documentation, who knows which one is which. (Especially, since it’s require.ErrorIs(t, actual, expect) but require.Equals(t, expect, actual) )

This is especially true, when one is coming from a language that incentivizes writing equality tests as constValue == testValue so that mistakenly only using one = will not produce an unexpectedly valid assignment, but an invalid unassignable left-hand value. In Go, you cannot put an assignment in a condition, and so there is no way to accidentally use a single = assignment like this. This idiom is probably the very reason why require.Equal orders its parameters in that order… an order that is discouraged in Go.

Instead, something like this (without the comments which are here for study, rather than for good code practice):

if got != int64(shortWrite) {
	// In the `require.Equals`, the two arguments are cast into `interface{}`,
	// which means you lose the same compile-time type safety you would normally have.
	// It’s way better to use the code the exact same as your caller would use the code,
	// with verbose `if err != nil { }` et al. included.
	
	// Note the order here, `if got != expected { … }`, and the message orders `got` then `expected`.
	t.Errorf("sftpFile.Write() = %d, but expected %d", got, shortWrite)
}

Also, we don’t want to require either of these tests anyways. We only want to assert it in this case. (Since both written and err have valid values, testing both of them every test makes sense. require is only appropriate when the test is a requirement for the remaining tests to have any meaningful result and/or not panic.