Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
> On Wed, Jun 08 2022, Johannes Schindelin via GitGitGadget wrote:
>
>> From: Johannes Schindelin <johannes.schindelin@xxxxxx>
>>
>> Git now shows better information in the GitHub workflow runs when a test
>> case failed.
>
> This commit message should be more on-point. "Git" isn't showing
> anything, and it's unclear that this is a regression fix for fc5a070f591
> (Merge branch 'js/ci-github-workflow-markup', 2022-06-07).
>
>> However, when a test case was implemented incorrectly and therefore
>> does not even run, nothing is shown.
>
> The *report* came about because of an incorrectly implemented test (of
> mine).
>
> But the real issue is that your recent change to the CI output is
> implemented in such a way as to hide entire classes of errors that we'd
> previously show.
Yup, I think we are all on the same page on that. And we also all
agree that CI output should help those developers who make mistakes
by making it easy to see them. The recent change may have been too
aggressive to hide stuff in the name of "newbie friendliness", which
may or may not have been a mismatch between what it claimed to aim
at and what it actually did, but let's not grumble too much about it
and move on in a more constructive way.
These fix-ups are to correct such earlier mistakes.
> I don't think we should call these "full logs" while the bug above
> remains unsolved. Perhaps "more verbose logs emitted when running with
> --github-workflow-markup, omit the flag to get the full *.out logs"?
Yup.
>> diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
>> index 3fa88b78b6d..cd1f52692a5 100644
>> --- a/.github/workflows/main.yml
>> +++ b/.github/workflows/main.yml
>> @@ -119,6 +119,10 @@ jobs:
>> - name: test
>> shell: bash
>> run: . /etc/profile && ci/run-test-slice.sh ${{matrix.nr}} 10
>> + - name: print test failures
>
> When ci/print-test-failures.sh was last in this file before 08dccc8fc1f
> (ci: make it easier to find failed tests' logs in the GitHub workflow,
> 2022-05-21) there was no "name" field, that's an unrelated change that
> shouldn't be part of a narrow regression fix.
>
>> + if: failure() && env.FAILED_TEST_ARTIFACTS != ''
>
> We likewise just had "if failure()" then, is the distinction different
> in all these cases?
>
>> + shell: bash
>
> ...and you've made every single one of them run with "bash" instead of
> the default shell, which is another "change while at it" that isn't
> discussed.
If it is so important to support all the other shells in the GitHub
workflows environment, we can discuss fix-up patches on top or
replacement patches, but does that really matter? If this were main
Makefile or ci/*.sh that are supposed to be usable by places other
than GitHub Actions environment we use for the CI there, of course
it would be worth to try being extra portable, but it may be even
beneficial to "fix" .github/workflows/* stuff, so that we won't have
to be affected by mistaken use of non-portable shell construct
written there, perhaps?
