Fix code and unit tests to successfully run on Go 1.10 #7610
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7610 +/- ##
==========================================
+ Coverage 31.41% 31.47% +0.05%
==========================================
Files 282 281 -1
Lines 42152 42027 -125
==========================================
- Hits 13244 13228 -16
+ Misses 27750 27637 -113
- Partials 1158 1162 +4
Continue to review full report at Codecov.
|
| copy(w, writers) | ||
| t := &multiWriter{writers: w} | ||
|
|
||
| t := &multiWriter{writers: writers} |
There was a problem hiding this comment.
huh, interesting. Why is this copy unnecessary?
There was a problem hiding this comment.
I guess the answer to that could just be "because it is, and it wasn't needed in the first place"
|
Probably want @matthewavery to have a look at this too |
| @@ -141,39 +120,35 @@ func (t *multiWriter) Close() error { | |||
| return nil | |||
| } | |||
|
|
|||
| // TODO: add a ReadFrom for more efficient copy | |||
There was a problem hiding this comment.
Why remove the TODO here? it is still a function that would be nice to have.
| if w == writer { | ||
| t.writers = append(t.writers[:i], t.writers[i+1:]...) | ||
| if verbose { | ||
| log.Debugf("[%p] removed writer - now %d writers", t, len(t.writers)) |
There was a problem hiding this comment.
having a verbose debug of every wrtier that is left after the removal of one is still useful. just in the case that something is getting removed when it should not(for whatever reason).
There was a problem hiding this comment.
woops meant this for 2 lines down. sorry.
| @@ -132,17 +132,19 @@ func TestSingleStripper(t *testing.T) { | |||
| } | |||
|
|
|||
| // make the buf just that little bit bigger to allow for errrors in the copy if they would occur | |||
| buf := make([]byte, size+10) | |||
| buf := make([]byte, size) | |||
There was a problem hiding this comment.
you need to remove the comment above if you've dropped the extra spacing. Why did you drop the extra spacing?
| n, err := tr.Read(buf) | ||
|
|
||
| if !assert.NoError(t, err, "No expected errors from file data copy") { | ||
| if !assert.Equal(t, n, size, "Expected file data size to match target generated size") { |
There was a problem hiding this comment.
There is a require that can be used instead of assert now, which does FailNow if the assertion fails. Simplifies the code significantly.
| if n != size { | ||
| panic(fmt.Sprintf("Failed to write all bytes: %d != %d", n, size)) | ||
| } | ||
| for i := 0; i < num; i++ { |
There was a problem hiding this comment.
Why are we removing the goroutine? The goroutine doesn't have that much of an effect in the test, but it's a better representation of how it gets used in reality.
| // Remove grabs the lock | ||
| t.Remove(w) | ||
|
|
||
| t.remove(w) |
There was a problem hiding this comment.
remove no longer grabs the lock - please make sure you update comments if you're changing behaviour as this gets very, very confusing.
|
|
||
| eof := 0 | ||
| // possibly want to add buffering or parallelize this | ||
| for _, w := range wTmp { | ||
| for _, w := range t.writers { | ||
| n, err = w.Write(p) |
There was a problem hiding this comment.
If I recall correctly this call can block depending on the Writer type - this is one of the reasons that we didn't hold the lock while performing the write.
What is the purpose and intent behind removing all of the parallelism from these tests? Generally that's where we would observe the most issues so this seems to be reducing the efficacy of the tests. And in this specific case, of the functional package code as well.
Fixes: #7609