From: Christian Couder <christian.couder@gmail.com>
Date: Fri, 03 Nov 2023 22:35:01 GMT
Subject: Re: [PATCH 4/5] ci: split out logic to set up failed test artifacts
Message-ID: <CAP8UFD25O9D1WL+CAQOcdqwuPsgdd6qOMNWuVtAw18U3RccYZQ@mail.gmail.com>
In-Reply-To: <4a864a1d174f7d4d36202a375302d450fbe9f2a3.1698305961.git.ps@pks.im>

On Thu, Oct 26, 2023 at 10:01 AM Patrick Steinhardt <ps@pks.im> wrote:
>
> We have some logic in place to create a directory with the output from
> failed tests, which will then subsequently be uploaded as CI artifact.
> We're about to add support for GitLab CI, which will want to reuse the
> logic.
>
> Split the logic into a separate function so that it is reusable.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  ci/lib.sh | 40 ++++++++++++++++++++++------------------
>  1 file changed, 22 insertions(+), 18 deletions(-)
>
> diff --git a/ci/lib.sh b/ci/lib.sh
> index 957fd152d9c..33005854520 100755
> --- a/ci/lib.sh
> +++ b/ci/lib.sh
> @@ -137,6 +137,27 @@ handle_failed_tests () {
>         return 1
>  }
>
> +create_failed_test_artifacts () {
> +       mkdir -p t/failed-test-artifacts
> +
> +       for test_exit in t/test-results/*.exit
> +       do
> +               test 0 != "$(cat "$test_exit")" || continue
> +
> +               test_name="${test_exit%.exit}"
> +               test_name="${test_name##*/}"
> +               printf "\\e[33m\\e[1m=== Failed test: ${test_name} ===\\e[m\\n"
> +               echo "The full logs are in the 'print test failures' step below."
> +               echo "See also the 'failed-tests-*' artifacts attached to this run."
> +               cat "t/test-results/$test_name.markup"
> +
> +               trash_dir="t/trash directory.$test_name"
> +               cp "t/test-results/$test_name.out" t/failed-test-artifacts/
> +               tar czf t/failed-test-artifacts/"$test_name".trash.tar.gz "$trash_dir"
> +       done
> +       return 1

Nit: I think it's more the responsibility of handle_failed_tests() to
`return 1` than of create_failed_test_artifacts(). I understand that
putting it into this new function means one more line that is common
to several CI tools though.

Otherwise everything in this series LGTM. Thanks!