Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2958913002:
isolate: pull exparchive filewalk code into its own func (Closed)
Descriptionisolate: pull exparchive filewalk code into its own func.
Also, add tests for the filewalking code.
BUG=692940
Review-Url: https://codereview.chromium.org/2958913002
Committed: https://github.com/luci/luci-go/commit/64dd531232d94c5351a573e78732d8fc84b28349
Patch Set 1 #Patch Set 2 : Add tests. #
Total comments: 17
Patch Set 3 : address review comments #Patch Set 4 : rename partitionWalker #
Total comments: 6
Patch Set 5 : address review comments #Messages
Total messages: 14 (5 generated)
Description was changed from ========== isolate: pull exparchive filewalk code into its own func BUG=692940 ========== to ========== isolate: pull exparchive filewalk code into its own func. Also, add tests for the filewalking code. BUG=692940 ==========
mcgreevy@chromium.org changed reviewers: + tansell@chromium.org
https://codereview.chromium.org/2958913002/diff/20001/client/cmd/isolate/exp_... File client/cmd/isolate/exp_archive.go (right): https://codereview.chromium.org/2958913002/diff/20001/client/cmd/isolate/exp_... client/cmd/isolate/exp_archive.go:92: type partitionWalker struct { partition is kind of a strange name. Maybe "groupByType" or something similar? https://codereview.chromium.org/2958913002/diff/20001/client/cmd/isolate/exp_... File client/cmd/isolate/exp_archive_test.go (right): https://codereview.chromium.org/2958913002/diff/20001/client/cmd/isolate/exp_... client/cmd/isolate/exp_archive_test.go:78: for _, tc := range []testCase{ I would have tested the following cases; * empty directory * non-existent directory * single file * multiple files in a directory * recursive * recursive with empty directory * zero sized file * something around symlinks * something around the error cases https://codereview.chromium.org/2958913002/diff/20001/client/cmd/isolate/exp_... client/cmd/isolate/exp_archive_test.go:135: Size: 1, Why are these size 1 / 2? https://codereview.chromium.org/2958913002/diff/20001/client/cmd/isolate/exp_... client/cmd/isolate/exp_archive_test.go:144: size: 3, Size 3? Is this the cumulative size / total size? https://codereview.chromium.org/2958913002/diff/20001/client/cmd/isolate/exp_... client/cmd/isolate/exp_archive_test.go:219: continue TestCases I don't quite get the usage of the continue here?
https://codereview.chromium.org/2958913002/diff/20001/client/cmd/isolate/exp_... File client/cmd/isolate/exp_archive.go (right): https://codereview.chromium.org/2958913002/diff/20001/client/cmd/isolate/exp_... client/cmd/isolate/exp_archive.go:92: type partitionWalker struct { On 2017/06/27 07:48:10, mithro wrote: > partition is kind of a strange name. Maybe "groupByType" or something similar? I'll think about this name and get back to this point after lunch. https://codereview.chromium.org/2958913002/diff/20001/client/cmd/isolate/exp_... File client/cmd/isolate/exp_archive_test.go (right): https://codereview.chromium.org/2958913002/diff/20001/client/cmd/isolate/exp_... client/cmd/isolate/exp_archive_test.go:78: for _, tc := range []testCase{ On 2017/06/27 07:48:10, mithro wrote: > I would have tested the following cases; Most of the tests that you have suggested below would be tests of filepath.Walk, rather than walkFn. What I am testing here is that walkFn correctly adheres to the behaviour expected by filepath.Walk. There is no way to mock out the filesystem used by filepath.Walk, and I don't want to hit the actual filesystem in these tests. I think testing the behaviour of walkFn is sufficient. Regarding particular suggestions: > * empty directory it's up to filepath.Walk to handle the contents of directories. walkFn has no idea whether a directory is empty or not. > * non-existent directory walkFn is only given directories which exist; testing non-existing directories would be a test of filepath.Walk. > * single file Done. > * multiple files in a directory This is covered by multiple tests below. > * recursive This would be a test of filepath.Walk. > * recursive with empty directory This would be a test of filepath.Walk. > * zero sized file Done. > * something around symlinks symlinks are covered. > * something around the error cases I've added a test for bad relative paths, which is the only case that wasn't already covered. https://codereview.chromium.org/2958913002/diff/20001/client/cmd/isolate/exp_... client/cmd/isolate/exp_archive_test.go:135: Size: 1, On 2017/06/27 07:48:10, mithro wrote: > Why are these size 1 / 2? When testing things that add up values, I tend to use powers of 2, since there's no way to combine them by some means other than the correct way to end up with the expected answer. For example, if both of these were size 10, then the expected total size would be 20, but that could be arrived at by adding the first file twice, rather than the first and second file. Using powers of 2 avoids this. https://codereview.chromium.org/2958913002/diff/20001/client/cmd/isolate/exp_... client/cmd/isolate/exp_archive_test.go:144: size: 3, On 2017/06/27 07:48:10, mithro wrote: > Size 3? > > Is this the cumulative size / total size? total size. https://codereview.chromium.org/2958913002/diff/20001/client/cmd/isolate/exp_... client/cmd/isolate/exp_archive_test.go:219: continue TestCases On 2017/06/27 07:48:10, mithro wrote: > I don't quite get the usage of the continue here? If we get an unexpected error from walkFunc, there is no point in checking got vs want below. This could be t.Fatalf, except this is a table-driven test and we still want to test the other test cases. So instead of using t.Fatalf, I use continue to skip to the next for loop iteration and run the next test case.
https://codereview.chromium.org/2958913002/diff/20001/client/cmd/isolate/exp_... File client/cmd/isolate/exp_archive.go (right): https://codereview.chromium.org/2958913002/diff/20001/client/cmd/isolate/exp_... client/cmd/isolate/exp_archive.go:92: type partitionWalker struct { On 2017/06/27 07:48:10, mithro wrote: > partition is kind of a strange name. Maybe "groupByType" or something similar? Do you mean it's strange because "partition" has another meaning in the context of filesystems? Here it's intended as a verb, i.e. meaning "divide into parts"; the files are partitioned into three subsets. I could make this clearer by calling it partitioningWalker. I don't really like "groupByType" because: * it's debatable whether large files are of a different type to small files * "groupByType" is a verb phrase, and I think structs names should be nouns or noun phrases. WDYT?
Thinking about it more, partition is telling me what is happening -- not really why / the reason it is happening. I can't really think of good name, so happy to just go with it if you can't either. https://codereview.chromium.org/2958913002/diff/20001/client/cmd/isolate/exp_... File client/cmd/isolate/exp_archive_test.go (right): https://codereview.chromium.org/2958913002/diff/20001/client/cmd/isolate/exp_... client/cmd/isolate/exp_archive_test.go:78: for _, tc := range []testCase{ On 2017/06/28 01:25:51, mcgreevy wrote: > On 2017/06/27 07:48:10, mithro wrote: > > I would have tested the following cases; > > Most of the tests that you have suggested below would be tests of filepath.Walk, > rather than walkFn. What I am testing here is that walkFn correctly adheres to > the behaviour expected by filepath.Walk. There is no way to mock out the > filesystem used by filepath.Walk, and I don't want to hit the actual filesystem > in these tests. I think testing the behaviour of walkFn is sufficient. > > Regarding particular suggestions: > > > * empty directory > > it's up to filepath.Walk to handle the contents of directories. walkFn has no > idea whether a directory is empty or not. If I understand correctly, walkFn is called with a directory object for every directory that is traversed? https://codereview.chromium.org/2958913002/diff/20001/client/cmd/isolate/exp_... client/cmd/isolate/exp_archive_test.go:135: Size: 1, On 2017/06/28 01:25:51, mcgreevy wrote: > On 2017/06/27 07:48:10, mithro wrote: > > Why are these size 1 / 2? > > When testing things that add up values, I tend to use powers of 2, since there's > no way to combine them by some means other than the correct way to end up with > the expected answer. For example, if both of these were size 10, then the > expected total size would be 20, but that could be arrived at by adding the > first file twice, rather than the first and second file. Using powers of 2 > avoids this. Minor comments; That makes a lot of sense, but with sizes 1, 2 the pattern could actually be mistake for 1, 2, 3, 4, 5 I would write them as 1**1, 1**2 or something if I was trying to be clear that is what I was trying to do. https://codereview.chromium.org/2958913002/diff/20001/client/cmd/isolate/exp_... client/cmd/isolate/exp_archive_test.go:219: continue TestCases On 2017/06/28 01:25:51, mcgreevy wrote: > On 2017/06/27 07:48:10, mithro wrote: > > I don't quite get the usage of the continue here? > > If we get an unexpected error from walkFunc, there is no point in checking got > vs want below. This could be t.Fatalf, except this is a table-driven test and > we still want to test the other test cases. So instead of using t.Fatalf, I use > continue to skip to the next for loop iteration and run the next test case. I think the biggest problem is that the label for the continue is a long way away from the place it is used. This could be solved by not defining the test cases in the for statement. I also dislike the use of the continue to a label. I think I would rewrite this in the following form; ----- testCases := []testCase{ } for _, tc := range testCases { err = nil for _, f := range tc.files { err = pw.walkFn(f.path, f.fbi, tc.walkFnErr) if err != nil { break } } if err != tc.wantErr { t.Errorf("partitioning deps(%s): walkFn got err %v; want %v", tc.name, err, tc.wantErr) continue } if got, want := pw.parts, tc.want; !reflect.DeepEqual(got, want) { t.Errorf("partitioning deps(%s): got %#v; want %#v", tc.name, got, want) } } ----- Ultimately you have done more go code then I have, so if you think this is more closer to the Go style I'm okay with going with your recommendation. https://codereview.chromium.org/2958913002/diff/60001/client/cmd/isolate/exp_... File client/cmd/isolate/exp_archive.go (right): https://codereview.chromium.org/2958913002/diff/60001/client/cmd/isolate/exp_... client/cmd/isolate/exp_archive.go:83: size int64 I think this should be named something like totalSize or totalBytes https://codereview.chromium.org/2958913002/diff/60001/client/cmd/isolate/exp_... client/cmd/isolate/exp_archive.go:102: archiveFiles itemGroup Maybe better to name something like "filesForArchive" to be clear that this isn't the name of the actual archive? https://codereview.chromium.org/2958913002/diff/60001/client/cmd/isolate/exp_... File client/cmd/isolate/exp_archive_test.go (right): https://codereview.chromium.org/2958913002/diff/60001/client/cmd/isolate/exp_... client/cmd/isolate/exp_archive_test.go:81: files: []file{ I think this test would technically have dir("/rootDir") before the file items below? None of these are going to hit the "xxx.IsDir() == true" on line 112.
PTAL https://codereview.chromium.org/2958913002/diff/20001/client/cmd/isolate/exp_... File client/cmd/isolate/exp_archive_test.go (right): https://codereview.chromium.org/2958913002/diff/20001/client/cmd/isolate/exp_... client/cmd/isolate/exp_archive_test.go:78: for _, tc := range []testCase{ On 2017/06/29 07:25:17, mithro wrote: > On 2017/06/28 01:25:51, mcgreevy wrote: > > On 2017/06/27 07:48:10, mithro wrote: > > > I would have tested the following cases; > > > > Most of the tests that you have suggested below would be tests of > filepath.Walk, > > rather than walkFn. What I am testing here is that walkFn correctly adheres to > > the behaviour expected by filepath.Walk. There is no way to mock out the > > filesystem used by filepath.Walk, and I don't want to hit the actual > filesystem > > in these tests. I think testing the behaviour of walkFn is sufficient. > > > > Regarding particular suggestions: > > > > > * empty directory > > > > it's up to filepath.Walk to handle the contents of directories. walkFn has no > > idea whether a directory is empty or not. > > If I understand correctly, walkFn is called with a directory object for every > directory that is traversed? walkFn is called with the *path*, and an os.FileInfo, and it can tell by calling FileInfo.IsDir whether the path is a directory, but it can not inspect the contents of the directory. All it can do is signal (via its return value) whether it wants filepath.Walk to enter the directory or not. https://codereview.chromium.org/2958913002/diff/20001/client/cmd/isolate/exp_... client/cmd/isolate/exp_archive_test.go:135: Size: 1, On 2017/06/29 07:25:17, mithro wrote: > On 2017/06/28 01:25:51, mcgreevy wrote: > > On 2017/06/27 07:48:10, mithro wrote: > > > Why are these size 1 / 2? > > > > When testing things that add up values, I tend to use powers of 2, since > there's > > no way to combine them by some means other than the correct way to end up with > > the expected answer. For example, if both of these were size 10, then the > > expected total size would be 20, but that could be arrived at by adding the > > first file twice, rather than the first and second file. Using powers of 2 > > avoids this. > > Minor comments; > > That makes a lot of sense, but with sizes 1, 2 the pattern could actually be > mistake for 1, 2, 3, 4, 5 > > I would write them as 1**1, 1**2 or something if I was trying to be clear that > is what I was trying to do. OK, done. https://codereview.chromium.org/2958913002/diff/20001/client/cmd/isolate/exp_... client/cmd/isolate/exp_archive_test.go:219: continue TestCases On 2017/06/29 07:25:17, mithro wrote: > On 2017/06/28 01:25:51, mcgreevy wrote: > > On 2017/06/27 07:48:10, mithro wrote: > > > I don't quite get the usage of the continue here? > > > > If we get an unexpected error from walkFunc, there is no point in checking got > > vs want below. This could be t.Fatalf, except this is a table-driven test and > > we still want to test the other test cases. So instead of using t.Fatalf, I > use > > continue to skip to the next for loop iteration and run the next test case. > > I think the biggest problem is that the label for the continue is a long way > away from the place it is used. This could be solved by not defining the test > cases in the for statement. > > I also dislike the use of the continue to a label. > > I think I would rewrite this in the following form; > ----- > > testCases := []testCase{ > } > > for _, tc := range testCases { > err = nil > for _, f := range tc.files { > err = pw.walkFn(f.path, f.fbi, tc.walkFnErr) > if err != nil { > break > } > } > > if err != tc.wantErr { > t.Errorf("partitioning deps(%s): walkFn got err %v; want %v", tc.name, err, > tc.wantErr) > continue > } > > if got, want := pw.parts, tc.want; !reflect.DeepEqual(got, want) { > t.Errorf("partitioning deps(%s): got %#v; want %#v", tc.name, got, want) > } > > } > ----- > > Ultimately you have done more go code then I have, so if you think this is more > closer to the Go style I'm okay with going with your recommendation. Looking at this more closely, I think that it is wrong to have a single "wantErr" that is compared to the return value of potentially multiple calls to walkFn. I'm pulling these cases into another test to fix that. I've moved the label down, as you suggested. As for removing the label, I think that it is more robust, when encountering an expected error here, to bail out of the test case immediately, rather than set up some state (namely, storing the error in an err variable) that then needs to be inspected again after breaking from the loop. It's analogous to an early return. Note, the label here is not being used with something unstructured like a goto, it's merely a way of indicating which level of for loop you wish to continue. I mentioned above that the continue is analogous to an early return, and in fact another way to address this would be to extract the entire body of the for loop into a helper function. Then the continue would actually become a return. That would be a good solution, except for the fact that the helper function would need access to the testCase type, which means that either: (a) the definition of the testCase type would need to be moved outside of TestWalkFn (not ideal, since no other code needs to know about it; also not the idiomatic way of writing table-driven tests), or (b) the helper function would need to be defined inside TestWalkFn. I think that would just overcomplicate things and make it harder to read. For these reasons, and the fact that labels are commonly used in this way in Go, I'm going to retain the use of the label. https://codereview.chromium.org/2958913002/diff/60001/client/cmd/isolate/exp_... File client/cmd/isolate/exp_archive.go (right): https://codereview.chromium.org/2958913002/diff/60001/client/cmd/isolate/exp_... client/cmd/isolate/exp_archive.go:83: size int64 On 2017/06/29 07:25:17, mithro wrote: > I think this should be named something like totalSize or totalBytes Done. https://codereview.chromium.org/2958913002/diff/60001/client/cmd/isolate/exp_... client/cmd/isolate/exp_archive.go:102: archiveFiles itemGroup On 2017/06/29 07:25:17, mithro wrote: > Maybe better to name something like "filesForArchive" to be clear that this > isn't the name of the actual archive? I'm not sure how this could be the "name" of the archive, but I've renamed it to "filesToArchive". https://codereview.chromium.org/2958913002/diff/60001/client/cmd/isolate/exp_... File client/cmd/isolate/exp_archive_test.go (right): https://codereview.chromium.org/2958913002/diff/60001/client/cmd/isolate/exp_... client/cmd/isolate/exp_archive_test.go:81: files: []file{ On 2017/06/29 07:25:18, mithro wrote: > I think this test would technically have dir("/rootDir") before the file items > below? None of these are going to hit the "xxx.IsDir() == true" on line 112. I don't know what you mean by "this test would technically have dir("/rootDir") before the file items below?". line 112 is covered by the "does nothing for directories" test case, now moved into its own test function.
lgtm
The CQ bit was checked by mcgreevy@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1498794056714900,
"parent_rev": "2224066a0cc0b0fa150c733a614116c87807637e", "commit_rev":
"64dd531232d94c5351a573e78732d8fc84b28349"}
Message was sent while issue was closed.
Description was changed from ========== isolate: pull exparchive filewalk code into its own func. Also, add tests for the filewalking code. BUG=692940 ========== to ========== isolate: pull exparchive filewalk code into its own func. Also, add tests for the filewalking code. BUG=692940 Review-Url: https://codereview.chromium.org/2958913002 Committed: https://github.com/luci/luci-go/commit/64dd531232d94c5351a573e78732d8fc84b28349 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001) as https://github.com/luci/luci-go/commit/64dd531232d94c5351a573e78732d8fc84b28349 |
