From: Justin Tobler Date: Mon, 09 Feb 2026 18:07:11 GMT Subject: Re: [PATCH 2/5] ci: don't skip smallest test slice in GitLab Message-ID: In-Reply-To: <20260209-b4-pks-ci-meson-improvements-v1-2-38444dec4874@pks.im> 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 > --- > .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