diff mbox series

[4/9] ci/run-build-and-tests: add some structure to the GitHub workflow output

Message ID 9333ba781b8240f704e739b00d274f8c3d887e39.1643050574.git.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series ci: make Git's GitHub workflow output much more helpful | expand

Commit Message

Johannes Schindelin Jan. 24, 2022, 6:56 p.m. UTC 
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.

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(-)

Comments

Phillip Wood Feb. 23, 2022, 12:13 p.m. UTC | #1 
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
Johannes Schindelin Feb. 25, 2022, 1:40 p.m. UTC | #2 
Hi Phillip,

On Wed, 23 Feb 2022, Phillip Wood wrote:

> On 24/01/2022 18:56, Johannes Schindelin via GitGitGadget wrote:
>
> > +# 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?

Oh yes, the comment needs to be updated. Thank you for pointing that out.

Ciao,
Dscho
diff mbox series

Patch

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
+
 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