Re: [PATCH 2/5] ci: don't skip smallest test slice in GitLab

[Justin Tobler <jltobler@gmail.com>]

On 26/02/09 05:56PM, Patrick Steinhardt wrote:
> The "ci/run-test-slice.sh" script can be used to slice up all of our
> tests into N pieces and then run each of them on a separate CI job.
> This is used by both GitLab and GitHub CI to speed up Windows tests,
> which would otherwise be painfully slow.
> 
> The infra itself is fueled by `test-tool path-utils slice-tests`. This
> tool receives as input an "offset" and a "stride" that can be combined
> to slice up tests. This framing can be misleading though: you are
> expected to pass a zero-based index as "offset", and the complete number
> of slices to the "stride". The latter makes sense, but it is somewhat
> surprising that the offset needs to be zero-based. And this is in fact
> biting us: while GitHub passes zero-based indices, GitLab passes
> `$CI_NODE_INDEX`, which is a one-based indice.
> 
> Ideally, we should have verification that the parameters make sense.
> And naturally, one would for example expect that it's an error to call
> the binary with an offset larger than the stride. But with the current
> framing as "offset" it's not even wrong to do so, as it is of course
> well-defined to start at a larger offset than the stride.

It was also suprising for me to see that the "offset" could be set to a
value higher than the stride. I can't see any reason that we would want
this to be the case.

> This means that we get this wrong on GitLab's CI, as we pass a one based
> index there, and this causes us to skip one of the tests. Interestingly,
> it's not the lexicographically first test that we skip. Instead, as we
> sort tests by size before slicing them, we skip the _smallest_ test.
> 
> Reframe the problem to instead talk about "slice number" and "total
> number of slices". For all of our use cases this is semantically
> equivalent, but it allows us to perform some verifications:
> 
>   - The total number of slices must be greater than 1.
> 
>   - The selected slice must be between 1 <= nr <= slices_total.

This seems reasonable to me.

> As the indices are now one-based it means that GitLab's CI is fixed.
> The GitHub workflow is updated accordingly.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  .github/workflows/main.yml |  2 +-
>  t/helper/test-path-utils.c | 18 ++++++++++++------
>  2 files changed, 13 insertions(+), 7 deletions(-)
> 
> diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
> index f2e93f5461..2b175dc5c6 100644
> --- a/.github/workflows/main.yml
> +++ b/.github/workflows/main.yml
> @@ -150,7 +150,7 @@ jobs:
>      - uses: git-for-windows/setup-git-for-windows-sdk@v1
>      - name: test
>        shell: bash
> -      run: . /etc/profile && ci/run-test-slice.sh ${{matrix.nr}} 10
> +      run: . /etc/profile && ci/run-test-slice.sh ${{ matrix.nr + 1 }} 10

Here the GitHub CI is updated to be one-based indexed. The GitLab CI is
already set up that way.

>      - name: print test failures
>        if: failure() && env.FAILED_TEST_ARTIFACTS != ''
>        shell: bash
> diff --git a/t/helper/test-path-utils.c b/t/helper/test-path-utils.c
> index f5f33751da..874542ec34 100644
> --- a/t/helper/test-path-utils.c
> +++ b/t/helper/test-path-utils.c
> @@ -477,14 +477,20 @@ int cmd__path_utils(int argc, const char **argv)
>  
>  	if (argc > 5 && !strcmp(argv[1], "slice-tests")) {
>  		int res = 0;
> -		long offset, stride, i;
> +		long slice, slices_total, i;
>  		struct string_list list = STRING_LIST_INIT_NODUP;
>  		struct stat st;
>  
> -		offset = strtol(argv[2], NULL, 10);
> -		stride = strtol(argv[3], NULL, 10);
> -		if (stride < 1)
> -			stride = 1;
> +		slices_total = strtol(argv[3], NULL, 10);
> +		if (slices_total < 1)
> +			die("there must be at least one slice, got '%s'",
> +			    argv[3]);

Here we validate the slices count is greater than one.

> +
> +		slice = strtol(argv[2], NULL, 10);
> +		if (1 > slice || slice > slices_total)
> +			die("slice must be in the range 1 <= slice <= %ld, got '%s'",
> +			    slices_total, argv[2]);

Here we validate the provided slice index is in the correct range.

> +
>  		for (i = 4; i < argc; i++)
>  			if (stat(argv[i], &st))
>  				res = error_errno("Cannot stat '%s'", argv[i]);
> @@ -492,7 +498,7 @@ int cmd__path_utils(int argc, const char **argv)
>  				string_list_append(&list, argv[i])->util =
>  					(void *)(intptr_t)st.st_size;
>  		QSORT(list.items, list.nr, cmp_by_st_size);
> -		for (i = offset; i < list.nr; i+= stride)
> +		for (i = slice - 1; i < list.nr; i+= slices_total)
>  			printf("%s\n", list.items[i].string);
>  
>  		return !!res;

This patch looks good.

-Justin