Issue 2992113002: isolate: test uploading of individual regular files

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+144 lines, -19 lines)			Patch
M	client/cmd/isolate/exp_archive.go	View	3	1 chunk	+0 lines, -9 lines	0 comments	Download
M	client/cmd/isolate/upload_tracker.go	View		4 chunks	+19 lines, -1 line	0 comments	Download
M	client/cmd/isolate/upload_tracker_test.go	View	1	9 chunks	+125 lines, -9 lines	0 comments	Download

Depends on Patchset:

Issue 2988133002 Patch 1

Dependent Patchsets:

Issue 2989173002 Patch 1

Messages

Total messages: 10 (6 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

mithro

CL description could use a little more detail.... Generally LGTM though. A few small random ...

3 years, 4 months ago (2017-08-01 04:45:43 UTC) #3

commit-bot: I haz the power

CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2992113002/60001

3 years, 4 months ago (2017-08-01 07:22:07 UTC) #6

commit-bot: I haz the power

Committed patchset #4 (id:60001) as https://github.com/luci/luci-go/commit/f91cf2864edce74d3d2fbddaccd27894c97b21a1

3 years, 4 months ago (2017-08-01 07:28:36 UTC) #9

mcgreevy

3 years, 4 months ago (2017-08-02 03:59:30 UTC) #10

Message was sent while issue was closed.

https://codereview.chromium.org/2992113002/diff/1/client/cmd/isolate/upload_t...
File client/cmd/isolate/upload_tracker_test.go (right):

https://codereview.chromium.org/2992113002/diff/1/client/cmd/isolate/upload_t...
client/cmd/isolate/upload_tracker_test.go:143: uploader.uploadFileCalls =
append(uploader.uploadFileCalls, uploaderUploadFileArgs{item, ps})
On 2017/08/01 04:45:42, mithro wrote:
> Do you ever call the done function? Don't you need too?

I didn't bother because the done function is only ever a one-line function which
prints a log line. But I suppose I should anyway.

Done.

https://codereview.chromium.org/2992113002/diff/1/client/cmd/isolate/upload_t...
client/cmd/isolate/upload_tracker_test.go:210: So(fos.writeFiles,
shouldResembleByteMap, map[string][]byte{"/a/isolatedPath": wantIsolJSON})
On 2017/08/01 04:45:42, mithro wrote:
> Thought to consider (no action needed in this CL..);
> 
> The only file we ever write is the isolated json output right? Is there any
> reason to have a generic writeFiles like functionality rather than doing
> something more around that?

I notice that after I uploaded this CL.  But then I realized I would still need
to store both the path and the json contents, which means I need either two
fields, or a new struct type, and I figured that this works just as well.

https://codereview.chromium.org/2992113002/diff/1/client/cmd/isolate/upload_t...
client/cmd/isolate/upload_tracker_test.go:277: // non-nil PushState means don't
skip the upload.
On 2017/08/01 04:45:42, mithro wrote:
> I'm a bit confused at how the push state is being used here?

The PushState is not really meant to be inspected by the client, so it doesn't
really matter what's in it.  What the client cares about is whether it is nil or
not.

So here I am returning a non-nil PushState from the checker to signify that the
file should be uploaded. 
 Later, I re-use that same PushState in the expected upload file args, because
the UploadTracker passes (to the uploader) the same PushState that it gets back
from the checker.

I am not returning a separate PushState each time the checker is called because
it would add extra complexity to the test that I don't think is worth it.

https://codereview.chromium.org/2992113002/diff/1/client/cmd/isolate/upload_t...
client/cmd/isolate/upload_tracker_test.go:324: wantIsolJSON :=
[]byte(fmt.Sprintf(wantIsolJSONTmpl, barHash, fooHash))
On 2017/08/01 04:45:42, mithro wrote:
> Is there any reason particular reason to define wantIsolJSONTmpl as a separate
> value rather than just putting it inside the Sprintf call?

Line length.

It's not Go style to split function parameter lists over multiple lines. So I am
doing this instead to avoid the Sprintf line getting too long.

https://codereview.chromium.org/2992113002/diff/1/client/cmd/isolate/upload_t...
client/cmd/isolate/upload_tracker_test.go:349: Mode:    os.FileMode(6),
On 2017/08/01 04:45:42, mithro wrote:
> You should be trying with different File modes maybe?

Done.

https://codereview.chromium.org/2992113002/diff/1/client/cmd/isolate/upload_t...
client/cmd/isolate/upload_tracker_test.go:359: func Int64(i int64) *int64 {
return &i }
On 2017/08/01 04:45:43, mithro wrote:
> Could you add a comment about why these casts are needed?

Done.

FYI they're not casts, they're just helpers to take the address of a literal
value, which is not otherwise allowed in Go  i.e. you can't do

var i *int = &10

but you can (with the above helper) do:

var i *int = Int(10)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages