Hi Ævar,
On 23 Feb 2022, at 16:50, Ævar Arnfjörð Bjarmason wrote:
> On Wed, Feb 23 2022, Junio C Hamano wrote:
>
>> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
>>
>>> This test was already a bit broken in needing the preceding tests, but
>>> it will break now if REFFILES isn't true, which you can reproduce
>>> e.g. with:
>>>
>>> ./t3903-stash.sh --run=1-16,18-50 -vixd
>>>
>>> Perhaps the least sucky solution to that is:
>>>
>>> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
>>> index ec9cc5646d6..1d11c9bda20 100755
>>> --- a/t/t3903-stash.sh
>>> +++ b/t/t3903-stash.sh
>>> @@ -205,13 +205,19 @@ test_expect_success 'drop stash reflog updates refs/stash with rewrite' '
>>> cat >expect <<-EOF &&
>>> $(test_oid zero) $oid
>>> EOF
>>> - test_cmp expect actual
>>> + test_cmp expect actual &&
>>> + >dropped-stash
>>> '
>>
>> If "git stash drop", invoked in earlier part of this test before the
>> precontext, fails, then test_cmp would fail and we leave
>> dropped-stash untouched, even though we did run "git stash drop"
>> already.
>
> Yes, that's an edge case that's exposed here, but which I thought wasn't
> worth bothering with. I.e. if you get such a failure on test N getting
> N+1 failing as well isn't that big of a deal.
>
> The big deal is rather that we know we're adding a REFFILES dependency
> to this, which won't run this at all, which will make the "stash pop"
> below fail.
>
>> Why does the next test need to depend on what has happened earlier?
>
> They don't need to, and ideally wouldn't, but most of our test suite has
> this issue already. Try e.g. running it with:
>
> prove t[0-9]*.sh :: --run=10-20 --immediate
>
> And for this particular file running e.g. this on master:
>
> ./t3903-stash.sh --run=1-10,30-40
>
> Will fail 7 tests in the 30-40 range.
>
> So while it's ideal that we can combine tests with arbitrary --run
> parameters, i.e. all tests would tear down fully, not depend on earlier
> tests etc. we're really far from that being the case in practice.
>
> So insisting on some general refactoring of this file as part of this
> series seems a bit overzelous, which is why I'm suggesting the bare
> minimum to expect and work around the inevitable REFFILES failure, as
> Han-Wen is actively working in that area.
Curious what your thoughts are on an effort to isolate these tests from each other.
I like your approach in t/t1417 in creating a test repo and copying it over each time.
Something like this?
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index ac345eced8cb..40254f8dc99c 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -41,7 +41,9 @@ diff_cmp () {
rm -f "$1.compare" "$2.compare"
}
-test_expect_success 'stash some dirty working directory' '
+test_expect_success 'setup' '
+ git init repo &&
+ cd repo &&
echo 1 >file &&
git add file &&
echo unrelated >other-file &&
@@ -54,48 +56,54 @@ test_expect_success 'stash some dirty working directory' '
test_tick &&
git stash &&
git diff-files --quiet &&
- git diff-index --cached --quiet HEAD
+ git diff-index --cached --quiet HEAD &&
+ cat >expect <<-EOF &&
+ diff --git a/file b/file
+ index 0cfbf08..00750ed 100644
+ --- a/file
+ +++ b/file
+ @@ -1 +1 @@
+ -2
+ +3
+ EOF
+ cd ../
'
-cat >expect <<EOF
-diff --git a/file b/file
-index 0cfbf08..00750ed 100644
---- a/file
-+++ b/file
-@@ -1 +1 @@
--2
-+3
-EOF
+test_stash () {
+ cp -R repo copy &&
+ cd copy &&
+ test_expect_success "$@" &&
+ cd ../ &&
+ rm -rf copy
+}
-test_expect_success 'parents of stash' '
+test_stash 'parents of stash' '
test $(git rev-parse stash^) = $(git rev-parse HEAD) &&
git diff stash^2..stash >output &&
diff_cmp expect output
'
Not sure if it's worth it though?
>
>>> test_expect_success 'stash pop' '
>>> git reset --hard &&
>>> git stash pop &&
>>> - test 9 = $(cat file) &&
>>> + if test -e dropped-stash
>>> + then
>>> + test 9 = $(cat file)
>>> + else
>>> + test 3 = $(cat file)
>>> + fi &&
>>> test 1 = $(git show :file) &&
>>> test 1 = $(git show HEAD:file) &&
>>> test 0 = $(git stash list | wc -l)
