On Wed, Mar 16, 2022 at 1:14 AM Derrick Stolee <derrickstolee@xxxxxxxxxx> wrote: > > Hello! Hi Derrick, > > > I'll answer your question "are the tests on the right track?" [1] inline > > with the tests here. > > In 't1092', I've tried to write test cases around some of the > > characteristics relevant to sparse checkout/sparse index. For example: > > > > - files inside vs. outside of sparse cone (e.g., 'deep/a' vs 'folder1/a') > > - "normal" directories vs. sparse directories (e.g., 'deep/' vs. 'folder1/') > > - directories inside a sparse directory vs. "toplevel" sparse directories > > (e.g., 'folder1/0/' vs. 'folder1/') > > - options that follow different code paths, especially if those code paths > > interact with the index differently (e.g., 'git reset --hard' vs 'git > > reset --mixed') > > - (probably not relevant for 'git mv') files with vs. without staged changes > > in the index > > > > I've found that exercising these characteristics provides good baseline > > coverage for a sparse index integration, not leaving any major gaps. I'll > > also typically add cases specific to any workarounds I need to add to a > > command (like for 'git read-tree --prefix' [2]). > > This, and other advice that Victoria mentions, are really > good points to keep in mind. > > > My recommendations: > > > > - add tests covering outside-of-sparse-cone 'mv' arguments > > - add tests covering 'mv' attempting to move directories (in-cone and > > sparse) > > - add some "test_must_fail" tests to see what happens when you do something > > "wrong", e.g. to try to overwrite a file without '-f' (I've found some > > really interesting issues in the past where you expect something to fail > > and it doesn't) > > - add 'git status --porcelain=v2' checks to confirm that the 'mv' worked the > > same across the different checkouts > > - remove multiples of test cases that test the same general behavior (e.g., > > 'git mv <in-cone file> <in-cone file>' only needs to be done once) > > - double-check whether '-v' and '-k' have the ability to affect > > full-checkout/sparse-checkout/sparse-index differently - if not, you > > probably don't need to test them > > > > Thanks for working on this, and I hope this helps! > > You mention in your cover letter that the ensure_not_expanded tests > are not added yet (same with performance tests). Now that you've > gotten feedback on this version of the patch, I might recommend the > organization you might want for a full series: > > 1. Add these 'mv' tests to t1092 _without_ the code change. These > tests should work when the index is expanded, and making the > code change to not expand the index shouldn't change the > behavior. > > 2. Add the performance test so we have a baseline to measure how > well 'mv' does in the normal case (and how it is slower when > expanding the index). > > 3. Make the code change and add the ensure_not_expanded test, > since the functionality from the tests added in (1) will not > change and we can report the results from the perf tests > added in (2). The only thing to test is the new, internal > behavior that the index is not expanded when doing these > actions. (Keep in mind that we expect the index to be > expanded for out-of-cone moves, but it's the in-cone moves > that we expect to not expand.) Thanks for the recommendations, they are really helpful! I will try to address them in the next patch :) -- Thanks & Regards, Shaoxuan
