On Mon, Mar 6, 2023 at 11:32 AM Glen Choo <chooglen@xxxxxxxxxx> wrote: > > Calvin Wan <calvinwan@xxxxxxxxxx> writes: > > > In preparation for later changes, move setup code into test_expect > > blocks. Smaller sections are moved into existing blocks, while larger > > sections with a standalone purpose are given their own new blocks. > > The changes where we moved lines outside of blocks into blocks without > changing them look good to me. > > > While at it, have tests clean up after themselves with > > test_when_finished > > I believe this came about as part of the discussion in > > https://lore.kernel.org/git/xmqqedqtbbf4.fsf@gitster.g > > I think it's good to have tests clean up after themselves, but I'm not > sure if that's what we're doing in all of these cases, see below. > > I'm leaving the diff header in place, since the two files have very > confusingly similar tests. > > > diff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh > > test_expect_success 'typechanged submodule(submodule->blob)' ' > > + test_when_finished rm -rf sm1 && > > git diff --submodule=log >actual && > > cat >expected <<-EOF && > > diff --git a/sm1 b/sm1 > > This hunk and the next... > > > @@ -212,9 +215,9 @@ test_expect_success 'typechanged submodule(submodule->blob)' ' > > test_cmp expected actual > > ' > > > > -rm -rf sm1 && > > -git checkout-index sm1 > > test_expect_success 'typechanged submodule(submodule->blob)' ' > > + test_when_finished rm -f sm1 && > > + git checkout-index sm1 && > > git diff-index -p --submodule=log HEAD >actual && > > cat >expected <<-EOF && > > Submodule sm1 $head4...0000000 (submodule deleted) > > were changed so that the "rm -rf" happens in the clean up phase of the > earlier test (test 14) instead of set up phase of the later test (test > 15). But, the "rm -rf" actually results in a _different_ state from > before 14, so it isn't actually cleaning up, it really is preparation > for 15's git checkout-index. > > You can observe this by running > > ./t4041-diff-submodule-option.sh --run=1-13,15 > > which fails as expected. On the other hand, it passes if we move the "rm > -rf" into test 15. > > Nearly all of the other test_when_finished here have the same problem, > where they 'clean up' state that wasn't changed in the same test body. I > believe they will show similar dependency issues, though I didn't go > through and test them all. Good catch. I'll go thru the rest of them and remove the dependency issues. > > > @@ -643,7 +643,6 @@ test_expect_success 'modified submodule contains modified content' ' > > diff_cmp expected actual > > ' > > > > -rm -rf sm1 > > test_expect_success 'deleted submodule' ' > > git diff-index -p --submodule=diff HEAD >actual && > > cat >expected <<-EOF && > > This one is fairly obvious, since the test says 'deleted submodule', but > we no longer delete the submodule in the setup. > > > @@ -779,9 +780,8 @@ test_expect_success 'diff --submodule=diff with .git file' ' > > diff_cmp expected actual > > ' > > > > -mv sm2 sm2-bak > > - > > test_expect_success 'deleted submodule with .git file' ' > > + mv sm2 sm2-bak && > > git diff-index -p --submodule=diff HEAD >actual && > > cat >expected <<-EOF && > > Submodule sm1 $head7...0000000 (submodule deleted) > > @@ -804,9 +804,9 @@ test_expect_success 'deleted submodule with .git file' ' > > diff_cmp expected actual > > ' > > > > -echo submodule-to-blob>sm2 > > - > > test_expect_success 'typechanged(submodule->blob) submodule with .git file' ' > > + test_when_finished "rm sm2 && mv sm2-bak sm2" && > > + echo submodule-to-blob>sm2 && > > git diff-index -p --submodule=diff HEAD >actual && > > cat >expected <<-EOF && > > Submodule sm1 $head7...0000000 (submodule deleted) > > @@ -836,9 +836,6 @@ test_expect_success 'typechanged(submodule->blob) submodule with .git file' ' > > diff_cmp expected actual > > ' > > > > -rm sm2 > > -mv sm2-bak sm2 > > This is the original case that Junio flagged, which I think is an almost > correct use of test_when_finished, since we do get back to an earlier > state before this string of tests, but not to the state before the > actual test with the test_when_finished. > > If we want to use test_when_finished here (which I think we do), we > should add another test_when_finished to remove the dependency between > the two tests. like so: > > test_expect_success 'deleted submodule with .git file' ' > + test_when_finished "mv sm2-bak sm2" && > mv sm2 sm2-bak && > git diff-index -p --submodule=diff HEAD >actual && > > ... > > test_expect_success 'typechanged(submodule->blob) submodule with .git file' ' > test_when_finished "rm sm2 && mv sm2-bak sm2" && > + mv sm2 sm2-bak && > > Currently, they're still dependent because one creates sm2-bak and the > other moves it back, but if we have each test restore sm2, there will be > no more dependency. > That all makes sense. Thanks for the recommendations
