git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 1/9] t/: new helper for tests that pass with ort but fail with recursive

From
Elijah Newren <newren@gmail.com>
Date
Oct 24, 2020, 16:53 UTC
Message-ID
<CABPp-BHgJrQMNEm7-y7nStVjcAedsNKH+bHNM9V34netTN+NTQ@mail.gmail.com>
In-Reply-To
<20201024104910.GA15823@danh.dev>

On Sat, Oct 24, 2020 at 3:49 AM Đoàn Trần Công Danh <congdanhqx@gmail.com> wrote:

Show 24 quoted lines
>
> On 2020-10-23 16:01:16+0000, Elijah Newren via GitGitGadget <gitgitgadget@gmail.com> wrote:
> > +test_expect_merge_algorithm () {
> > +     status_for_recursive=$1
> > +     shift
> > +     status_for_ort=$1
> > +     shift
> > +
> > +     if test "$GIT_TEST_MERGE_ALGORITHM" = ort
> > +     then
> > +             test_expect_${status_for_ort} "$@"
> > +     else
> > +             test_expect_${status_for_recursive} "$@"
> > -test_expect_failure 'check symlink modify/modify' '
> > +test_expect_merge_algorithm failure success 'check symlink modify/modify' '
>
> I find this series of "failure success" hard to decode without
> understanding what it would be, then I need to keep rememberring which
> status is corresponding with with algorithm.
>
> Perhaps this patch is a bit easier to read. This is largely based on
> your patch. (I haven't read other patches, yet).
>
> What do you think?

It is easier to read and I think something along these lines would make a lot of sense if this weren't a transient change (the idea is to eventually drop the recursive backend in favor of ort, and then these can all switch to just using test_expect_success). Maybe it still makes sense to make further changes here anyway, but if we do go this route, there are 1-2 things we can/should change:

First, while a lot of my contributions aren't that important, and the new test_expect_* function certainly falls in that category, one of the driving goals behind a new merge algorithm was fixing up edge and corner cases that were just too problematic in the recursive backend. Thus, the patch where I get to flip the test expectation is one that I care about more than most out of the (I'm guessing on this number) 100+ patches that will be part of this new merge algorithm. Having you take over ownership of that patch thus isn't right; we should instead keep my original patch and apply your suggested changes on top (or have a patch from you introducing a new function first, and then have a patch from me using it to flip test expectations on top).

Second, I think that lines like
    test_expect_merge_success recursive=failure ...
read like a contradiction and are also confusing.  I think it'd be
better if it read something like
    test_expect_merge recursive=failure ort=success ...
or something along those lines.
Show 257 quoted lines
> -------------8<------------
> From: =?UTF-8?q?=C4=90o=C3=A0n=20Tr=E1=BA=A7n=20C=C3=B4ng=20Danh?=
>  <congdanhqx@gmail.com>
> Date: Sat, 24 Oct 2020 17:41:02 +0700
> Subject: [PATCH] t/: new helper for testing merge that allow failure for some
>  algorithm
>
> There are a number of tests that the "recursive" backend does not handle
> correctly but which the redesign in "ort" will.
>
> Add a new helper in lib-merge.sh for selecting a different test expectation
> based on the setting of GIT_TEST_MERGE_ALGORITHM, and use it in various
> testcases to document that we expect them to be success by default
> but failure with certain algorithm.
>
> Signed-off-by: Đoàn Trần Công Danh <congdanhqx@gmail.com>
> ---
>  t/lib-merge.sh                         | 19 +++++++++++++++++++
>  t/t6416-recursive-corner-cases.sh      | 11 ++++++-----
>  t/t6422-merge-rename-corner-cases.sh   |  7 ++++---
>  t/t6423-merge-rename-directories.sh    | 13 +++++++------
>  t/t6426-merge-skip-unneeded-updates.sh |  3 ++-
>  t/t6430-merge-recursive.sh             |  3 ++-
>  6 files changed, 40 insertions(+), 16 deletions(-)
>  create mode 100644 t/lib-merge.sh
>
> diff --git a/t/lib-merge.sh b/t/lib-merge.sh
> new file mode 100644
> index 0000000000..efd8b9615c
> --- /dev/null
> +++ b/t/lib-merge.sh
> @@ -0,0 +1,19 @@
> +# Helper functions used by merge tests.
> +
> +test_expect_merge_success() {
> +       exception="$1"
> +       : "${GIT_TEST_MERGE_ALGORITHM:=recursive}"
> +       case ",$exception," in
> +       *,$GIT_TEST_MERGE_ALGORITHM=failure,*)
> +               shift
> +               test_expect_failure "$@" ;;
> +       *,$GIT_TEST_MERGE_ALGORITHM=*)
> +               BUG "exception must be failure only" ;;
> +       *=failure,)
> +               shift
> +               test_expect_success "$@" ;;
> +       *)
> +               # No exception
> +               test_expect_success "$@" ;;
> +       esac
> +}
> diff --git a/t/t6416-recursive-corner-cases.sh b/t/t6416-recursive-corner-cases.sh
> index fd98989b14..d13c1afb4a 100755
> --- a/t/t6416-recursive-corner-cases.sh
> +++ b/t/t6416-recursive-corner-cases.sh
> @@ -3,6 +3,7 @@
>  test_description='recursive merge corner cases involving criss-cross merges'
>
>  . ./test-lib.sh
> +. "$TEST_DIRECTORY"/lib-merge.sh
>
>  #
>  #  L1  L2
> @@ -1069,7 +1070,7 @@ test_expect_success 'setup symlink modify/modify' '
>         )
>  '
>
> -test_expect_failure 'check symlink modify/modify' '
> +test_expect_merge_success recursive=failure 'check symlink modify/modify' '
>         (
>                 cd symlink-modify-modify &&
>
> @@ -1135,7 +1136,7 @@ test_expect_success 'setup symlink add/add' '
>         )
>  '
>
> -test_expect_failure 'check symlink add/add' '
> +test_expect_merge_success recursive=failure 'check symlink add/add' '
>         (
>                 cd symlink-add-add &&
>
> @@ -1223,7 +1224,7 @@ test_expect_success 'setup submodule modify/modify' '
>         )
>  '
>
> -test_expect_failure 'check submodule modify/modify' '
> +test_expect_merge_success recursive=failure 'check submodule modify/modify' '
>         (
>                 cd submodule-modify-modify &&
>
> @@ -1311,7 +1312,7 @@ test_expect_success 'setup submodule add/add' '
>         )
>  '
>
> -test_expect_failure 'check submodule add/add' '
> +test_expect_merge_success recursive=failure 'check submodule add/add' '
>         (
>                 cd submodule-add-add &&
>
> @@ -1386,7 +1387,7 @@ test_expect_success 'setup conflicting entry types (submodule vs symlink)' '
>         )
>  '
>
> -test_expect_failure 'check conflicting entry types (submodule vs symlink)' '
> +test_expect_merge_success recursive=failure 'check conflicting entry types (submodule vs symlink)' '
>         (
>                 cd submodule-symlink-add-add &&
>
> diff --git a/t/t6422-merge-rename-corner-cases.sh b/t/t6422-merge-rename-corner-cases.sh
> index 3375eaf4e7..0c8eb4df8a 100755
> --- a/t/t6422-merge-rename-corner-cases.sh
> +++ b/t/t6422-merge-rename-corner-cases.sh
> @@ -4,6 +4,7 @@ test_description="recursive merge corner cases w/ renames but not criss-crosses"
>  # t6036 has corner cases that involve both criss-cross merges and renames
>
>  . ./test-lib.sh
> +. "$TEST_DIRECTORY"/lib-merge.sh
>
>  test_setup_rename_delete_untracked () {
>         test_create_repo rename-delete-untracked &&
> @@ -878,7 +879,7 @@ test_setup_rad () {
>         )
>  }
>
> -test_expect_failure 'rad-check: rename/add/delete conflict' '
> +test_expect_merge_success recursive=failure 'rad-check: rename/add/delete conflict' '
>         test_setup_rad &&
>         (
>                 cd rad &&
> @@ -951,7 +952,7 @@ test_setup_rrdd () {
>         )
>  }
>
> -test_expect_failure 'rrdd-check: rename/rename(2to1)/delete/delete conflict' '
> +test_expect_merge_success recursive=failure 'rrdd-check: rename/rename(2to1)/delete/delete conflict' '
>         test_setup_rrdd &&
>         (
>                 cd rrdd &&
> @@ -1040,7 +1041,7 @@ test_setup_mod6 () {
>         )
>  }
>
> -test_expect_failure 'mod6-check: chains of rename/rename(1to2) and rename/rename(2to1)' '
> +test_expect_merge_success recursive=failure 'mod6-check: chains of rename/rename(1to2) and rename/rename(2to1)' '
>         test_setup_mod6 &&
>         (
>                 cd mod6 &&
> diff --git a/t/t6423-merge-rename-directories.sh b/t/t6423-merge-rename-directories.sh
> index 06b46af765..249fbb6853 100755
> --- a/t/t6423-merge-rename-directories.sh
> +++ b/t/t6423-merge-rename-directories.sh
> @@ -26,6 +26,7 @@ test_description="recursive merge with directory renames"
>  #                     files that might be renamed into each other's paths.)
>
>  . ./test-lib.sh
> +. "$TEST_DIRECTORY"/lib-merge.sh
>
>
>  ###########################################################################
> @@ -1339,7 +1340,7 @@ test_setup_6b1 () {
>         )
>  }
>
> -test_expect_failure '6b1: Same renames done on both sides, plus another rename' '
> +test_expect_merge_success recursive=failure '6b1: Same renames done on both sides, plus another rename' '
>         test_setup_6b1 &&
>         (
>                 cd 6b1 &&
> @@ -1412,7 +1413,7 @@ test_setup_6b2 () {
>         )
>  }
>
> -test_expect_failure '6b2: Same rename done on both sides' '
> +test_expect_merge_success recursive=failure '6b2: Same rename done on both sides' '
>         test_setup_6b2 &&
>         (
>                 cd 6b2 &&
> @@ -3471,7 +3472,7 @@ test_setup_10e () {
>         )
>  }
>
> -test_expect_failure '10e: Does git complain about untracked file that is not really in the way?' '
> +test_expect_merge_success recursive=failure '10e: Does git complain about untracked file that is not really in the way?' '
>         test_setup_10e &&
>         (
>                 cd 10e &&
> @@ -4104,7 +4105,7 @@ test_setup_12b1 () {
>         )
>  }
>
> -test_expect_failure '12b1: Moving two directory hierarchies into each other' '
> +test_expect_merge_success recursive=failure '12b1: Moving two directory hierarchies into each other' '
>         test_setup_12b1 &&
>         (
>                 cd 12b1 &&
> @@ -4272,7 +4273,7 @@ test_setup_12c1 () {
>         )
>  }
>
> -test_expect_failure '12c1: Moving one directory hierarchy into another w/ content merge' '
> +test_expect_merge_success recursive=failure '12c1: Moving one directory hierarchy into another w/ content merge' '
>         test_setup_12c1 &&
>         (
>                 cd 12c1 &&
> @@ -4632,7 +4633,7 @@ test_setup_12f () {
>         )
>  }
>
> -test_expect_failure '12f: Trivial directory resolve, caching, all kinds of fun' '
> +test_expect_merge_success recursive=failure '12f: Trivial directory resolve, caching, all kinds of fun' '
>         test_setup_12f &&
>         (
>                 cd 12f &&
> diff --git a/t/t6426-merge-skip-unneeded-updates.sh b/t/t6426-merge-skip-unneeded-updates.sh
> index 699813671c..8510d4da8b 100755
> --- a/t/t6426-merge-skip-unneeded-updates.sh
> +++ b/t/t6426-merge-skip-unneeded-updates.sh
> @@ -23,6 +23,7 @@ test_description="merge cases"
>  #                     files that might be renamed into each other's paths.)
>
>  . ./test-lib.sh
> +. "$TEST_DIRECTORY"/lib-merge.sh
>
>
>  ###########################################################################
> @@ -666,7 +667,7 @@ test_setup_4a () {
>  #   correct requires doing the merge in-memory first, then realizing that no
>  #   updates to the file are necessary, and thus that we can just leave the path
>  #   alone.
> -test_expect_failure '4a: Change on A, change on B subset of A, dirty mods present' '
> +test_expect_merge_success recursive=failure '4a: Change on A, change on B subset of A, dirty mods present' '
>         test_setup_4a &&
>         (
>                 cd 4a &&
> diff --git a/t/t6430-merge-recursive.sh b/t/t6430-merge-recursive.sh
> index a328260d42..4795a7abd0 100755
> --- a/t/t6430-merge-recursive.sh
> +++ b/t/t6430-merge-recursive.sh
> @@ -3,6 +3,7 @@
>  test_description='merge-recursive backend test'
>
>  . ./test-lib.sh
> +. "$TEST_DIRECTORY"/lib-merge.sh
>
>  test_expect_success 'setup 1' '
>
> @@ -641,7 +642,7 @@ test_expect_success 'merge-recursive copy vs. rename' '
>         test_cmp expected actual
>  '
>
> -test_expect_failure 'merge-recursive rename vs. rename/symlink' '
> +test_expect_merge_success recursive=failure 'merge-recursive rename vs. rename/symlink' '
>
>         git checkout -f rename &&
>         git merge rename-ln &&
> --
> 2.29.0.rc1
Previous: Đoàn Trần Công DanhNext: Đoàn Trần Công Danh
Message 10 of 31 in “Support both merge backends in the testsuite, via environment variable”
  1. 0/9 Support both merge backends in the testsuite, via environment variableElijah Newren via GitGitGadget, Oct 23, 2020
  2. 2/9 merge tests: expect improved directory/file conflict handling in ortElijah Newren via GitGitGadget, Oct 23, 2020
  3. Elijah NewrenOct 23, 2020
  4. 3/9 t6416: correct expectation for rename/rename(1to2) + directory/fileElijah Newren via GitGitGadget, Oct 23, 2020
  5. 1/9 t/: new helper for tests that pass with ort but fail with recursiveElijah Newren via GitGitGadget, Oct 23, 2020
  6. Junio C HamanoOct 23, 2020
  7. Elijah NewrenOct 23, 2020
  8. Elijah NewrenOct 23, 2020
  9. Đoàn Trần Công DanhOct 24, 2020
  10. Elijah NewrenOct 24, 2020
  11. Đoàn Trần Công DanhOct 25, 2020
  12. Elijah NewrenOct 26, 2020
  13. Junio C HamanoOct 26, 2020
  14. 4/9 t6404, t6423: expect improved rename/delete handling in ort backendElijah Newren via GitGitGadget, Oct 23, 2020
  15. 5/9 t6423: expect improved conflict markers labels in the ort backendElijah Newren via GitGitGadget, Oct 23, 2020
  16. 6/9 merge tests: expect slight differences in output for recursive vs. ortElijah Newren via GitGitGadget, Oct 23, 2020
  17. Elijah NewrenOct 24, 2020
  18. 9/9 t6423: add more details about direct resolution of directoriesElijah Newren via GitGitGadget, Oct 23, 2020
  19. Elijah NewrenOct 23, 2020
  20. 7/9 t6423, t6436: note improved ort handling with dirty filesElijah Newren via GitGitGadget, Oct 23, 2020
  21. 8/9 t6423: note improved ort handling with untracked filesElijah Newren via GitGitGadget, Oct 23, 2020
  22. 0/9 Support both merge backends in the testsuite, via environment variableElijah Newren via GitGitGadget, Oct 26, 2020
  23. 7/9 t6423, t6436: note improved ort handling with dirty filesElijah Newren via GitGitGadget, Oct 26, 2020
  24. 9/9 t6423: add more details about direct resolution of directoriesElijah Newren via GitGitGadget, Oct 26, 2020
  25. 8/9 t6423: note improved ort handling with untracked filesElijah Newren via GitGitGadget, Oct 26, 2020
  26. 6/9 merge tests: expect slight differences in output for recursive vs. ortElijah Newren via GitGitGadget, Oct 26, 2020
  27. 5/9 t6423: expect improved conflict markers labels in the ort backendElijah Newren via GitGitGadget, Oct 26, 2020
  28. 4/9 t6404, t6423: expect improved rename/delete handling in ort backendElijah Newren via GitGitGadget, Oct 26, 2020
  29. 2/9 merge tests: expect improved directory/file conflict handling in ortElijah Newren via GitGitGadget, Oct 26, 2020
  30. 1/9 t/: new helper for tests that pass with ort but fail with recursiveElijah Newren via GitGitGadget, Oct 26, 2020
  31. 3/9 t6416: correct expectation for rename/rename(1to2) + directory/fileElijah Newren via GitGitGadget, Oct 26, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.