From: Phillip Wood <phillip.wood123@gmail.com>
Date: Wed, 23 Feb 2022 12:13:51 GMT
Subject: Re: [PATCH 4/9] ci/run-build-and-tests: add some structure to the GitHub workflow output
Message-ID: <f73586a9-e9f1-aa1d-ac22-8102e6583e2b@gmail.com>
In-Reply-To: <9333ba781b8240f704e739b00d274f8c3d887e39.1643050574.git.gitgitgadget@gmail.com>

Hi Dscho

On 24/01/2022 18:56, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
> 
> The current output of Git's GitHub workflow can be quite confusing,
> especially for contributors new to the project.
> 
> To make it more helpful, let's introduce some collapsible grouping.
> Initially, readers will see the high-level view of what actually
> happened (did the build fail, or the test suite?). To drill down, the
> respective group can be expanded.
> 
> Note: sadly, workflow output currently cannot contain any nested groups
> (see https://github.com/actions/runner/issues/802 for details),
> therefore we take pains to ensure to end any previous group before
> starting a new one.

Thanks for working on this, I find it makes it much easier to get to the 
information I need when a test fails. I'm not familiar with github's 
grouping but I noticed something below I didn't understand.

> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
>   ci/lib.sh                 | 55 ++++++++++++++++++++++++++++++++++-----
>   ci/run-build-and-tests.sh |  4 +--
>   ci/run-test-slice.sh      |  2 +-
>   3 files changed, 51 insertions(+), 10 deletions(-)
> 
> diff --git a/ci/lib.sh b/ci/lib.sh
> index 2b2c0932320..4ed8f40ab02 100755
> --- a/ci/lib.sh
> +++ b/ci/lib.sh
> @@ -1,5 +1,49 @@
>   # Library of functions shared by all CI scripts
>   
> +if test true != "$GITHUB_ACTIONS"
> +then
> +	begin_group () { :; }
> +	end_group () { :; }
> +
> +	group () {
> +		shift
> +		"$@"
> +	}
> +	set -x
> +else
> +	begin_group () {
> +		need_to_end_group=t
> +		echo "::group::$1" >&2
> +		set -x
> +	}
> +
> +	end_group () {
> +		test -n "$need_to_end_group" || return 0
> +		set +x
> +		need_to_end_group=
> +		echo '::endgroup::' >&2
> +	}
> +	trap end_group EXIT
> +
> +	group () {
> +		set +x
> +		begin_group "$1"
> +		shift
> +		"$@"
> +		res=$?
> +		end_group
> +		return $res
> +	}
> +
> +	begin_group "CI setup"
> +fi
> +
> +# Set 'exit on error' for all CI scripts to let the caller know that
> +# something went wrong.
> +# Set tracing executed commands, primarily setting environment variables
> +# and installing dependencies.
> +set -e

The comment is moved unchanged but the set command has lost the "-x". We 
now have several "set -x" commands in the functions above and one below 
"end_group" lower down. Does the comment need updating as we are not 
enabling the tracing of executed commands here anymore?

Best Wishes

Phillip

>   skip_branch_tip_with_tag () {
>   	# Sometimes, a branch is pushed at the same time the tag that points
>   	# at the same commit as the tip of the branch is pushed, and building
> @@ -88,12 +132,6 @@ export TERM=${TERM:-dumb}
>   # Clear MAKEFLAGS that may come from the outside world.
>   export MAKEFLAGS=
>   
> -# Set 'exit on error' for all CI scripts to let the caller know that
> -# something went wrong.
> -# Set tracing executed commands, primarily setting environment variables
> -# and installing dependencies.
> -set -ex
> -
>   if test -n "$SYSTEM_COLLECTIONURI" || test -n "$SYSTEM_TASKDEFINITIONSURI"
>   then
>   	CI_TYPE=azure-pipelines
> @@ -138,7 +176,7 @@ then
>   			test_name="${test_exit%.exit}"
>   			test_name="${test_name##*/}"
>   			printf "\\e[33m\\e[1m=== Failed test: ${test_name} ===\\e[m\\n"
> -			cat "t/test-results/$test_name.out"
> +			group "Failed test: $test_name" cat "t/test-results/$test_name.out"
>   
>   			trash_dir="t/trash directory.$test_name"
>   			cp "t/test-results/$test_name.out" t/failed-test-artifacts/
> @@ -234,3 +272,6 @@ linux-leaks)
>   esac
>   
>   MAKEFLAGS="$MAKEFLAGS CC=${CC:-cc}"
> +
> +end_group
> +set -x
> diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
> index e49f9eaa8c0..5516f45f7fe 100755
> --- a/ci/run-build-and-tests.sh
> +++ b/ci/run-build-and-tests.sh
> @@ -48,10 +48,10 @@ esac
>   # Any new "test" targets should not go after this "make", but should
>   # adjust $MAKE_TARGETS. Otherwise compilation-only targets above will
>   # start running tests.
> -make
> +group Build make
>   if test -n "$run_tests"
>   then
> -	make test ||
> +	group "Run tests" make test ||
>   	handle_failed_tests
>   fi
>   check_unignored_build_artifacts
> diff --git a/ci/run-test-slice.sh b/ci/run-test-slice.sh
> index 63358c23e11..a3c67956a8d 100755
> --- a/ci/run-test-slice.sh
> +++ b/ci/run-test-slice.sh
> @@ -10,7 +10,7 @@ windows*) cmd //c mklink //j t\\.prove "$(cygpath -aw "$cache_dir/.prove")";;
>   *) ln -s "$cache_dir/.prove" t/.prove;;
>   esac
>   
> -make --quiet -C t T="$(cd t &&
> +group "Run tests" make --quiet -C t T="$(cd t &&
>   	./helper/test-tool path-utils slice-tests "$1" "$2" t[0-9]*.sh |
>   	tr '\n' ' ')" ||
>   handle_failed_tests