From: Johannes Schindelin <Johannes.Schindelin@gmx.de>
To: Victoria Dye <vdye@github.com>
Cc: "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>,
git@vger.kernel.org, "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
"Phillip Wood" <phillip.wood123@gmail.com>
Subject: Re: [PATCH v2 1/5] cmake: make it easier to diagnose regressions in CTest runs
Date: Tue, 18 Oct 2022 16:02:50 +0200 (CEST) [thread overview]
Message-ID: <29r609np-s0r1-r904-16o3-6918930o2ror@tzk.qr> (raw)
In-Reply-To: <3df77ffd-85a2-3a54-9005-34a24ec6e82d@github.com>
Hi Victoria,
On Wed, 7 Sep 2022, Victoria Dye wrote:
> Johannes Schindelin via GitGitGadget wrote:
> > From: Johannes Schindelin <johannes.schindelin@gmx.de>
> >
> > When a test script fails in Git's test suite, the usual course of action
> > is to re-run it using options to increase the verbosity of the output,
> > e.g. `-v` and `-x`.
> >
> > Like in Git's CI runs, when running the tests in Visual Studio via the
> > CTest route, it is cumbersome or at least requires a very unintuitive
> > approach to pass options to the test scripts.
>
> At first I wondered whether there's a way to make arg specification more
> intuitive, rather than silently changing defaults. Unfortunately, it looks
> like even in the latest versions of CTest don't really support passing
> arguments through to tests [1] (and the workarounds are unpleasant at best).
>
> But then, you mentioned that there *is* a cumbersome/unintuitive approach to
> passing the options; what approach were you thinking?
Thank you for pointing out that I assumed this to be more obvious than it
actually is.
The cumbersome way is to edit this part of the CMake definition
(https://github.com/git/git/blob/v2.38.0/contrib/buildsystems/CMakeLists.txt#L1091-L1096):
#test
foreach(tsh ${test_scipts})
add_test(NAME ${tsh}
COMMAND ${SH_EXE} ${tsh}
WORKING_DIRECTORY
${CMAKE_SOURCE_DIR}/t)
endforeach()
To pass an option like `-v --run=1,29`, the user would have to edit the
line `COMMAND ${SH_EXE} ${tsh}`, appending the options, and then
re-configure the CMake cache. And then not forget to _only_ run the test
script in question because those options do not make sense for the other
tests. And then not forget to undo the changes and re-configure the CMake
cache.
> [1] https://gitlab.kitware.com/cmake/cmake/-/issues/20470
>
> >
> > So let's just pass those options by default: This will not clutter any
> > output window but the log that is written to a log file will have
> > information necessary to figure out test failures.
>
> Makes sense, I don't see any harm in providing more verbose output by
> default here.
>
> >
> > While at it, also imitate what the Windows jobs in Git's CI runs do to
> > accelerate running the test scripts: pass the `--no-bin-wrappers` and
> > `--no-chain-lint` options. This makes the test runs noticeably faster
> > because the `bin-wrappers/` scripts as well as the `chain-lint` code
> > make heavy use of POSIX shell scripting, which is really, really slow on
> > Windows due to the need to emulate POSIX behavior via the MSYS2 runtime.
>
> I'm a bit more hesitant on including these. I see how the performance
> benefit (on Windows in particular) would make typical user experience nicer.
> But, if someone develops locally with '--no-chain-lint' specified, they'll
> be much more likely to miss a broken && chain (personally, I get caught by
> chain lint errors *all the time* when I'm adding/updating tests) and cause
> unnecessary churn in their patch submissions. So, my recommendation would be
> to drop '--no-chain-lint' here (unless it's less important to the average
> developer than I think it is).
>
> It's also possible that '--no-bin-wrappers' does something weird with their
> local installation of Git but I think it's safe enough to make the default
> if the performance gain is substantial.
>
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > contrib/buildsystems/CMakeLists.txt | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/contrib/buildsystems/CMakeLists.txt b/contrib/buildsystems/CMakeLists.txt
> > index 1b23f2440d8..4aee1e24342 100644
> > --- a/contrib/buildsystems/CMakeLists.txt
> > +++ b/contrib/buildsystems/CMakeLists.txt
> > @@ -1088,7 +1088,7 @@ file(GLOB test_scipts "${CMAKE_SOURCE_DIR}/t/t[0-9]*.sh")
> > #test
> > foreach(tsh ${test_scipts})
> > add_test(NAME ${tsh}
> > - COMMAND ${SH_EXE} ${tsh}
> > + COMMAND ${SH_EXE} ${tsh} --no-bin-wrappers --no-chain-lint -vx
>
> As you have it here, I don't think there's a way for a user to override
> these defaults (unless there's something about the manual workaround you
> mentioned earlier that allows it). Since a user could feasibly want to set
> their own options, could you add a build variable to CMakeLists.txt like
> 'GIT_TEST_OPTS'? You could use it to set the default options you have here,
> but a user could still specify their own value at build time to override.
Indeed, there is no way to override this, unless the user edits the
(Git-tracked) `CMakeLists.txt` file and then re-configures the cache.
In my experience, the most common scenario where a developer needs to pass
options to test script is when they need to restrict execution to
individual test cases via, say, `--run=1,29`.
Such a use case cannot be covered via the CMake approach because it is not
fine-grained enough: any options the user could set would apply to the
entire test suite.
I added a comment to the commit message advising users to run the test
script manually via Git Bash instead, if they wish to pass a different set
of command-line options to the test script.
Thanks,
Dscho
>
> > WORKING_DIRECTORY ${CMAKE_SOURCE_DIR}/t)
> > endforeach()
> >
>
>
next prev parent reply other threads:[~2022-10-18 14:48 UTC|newest]
Thread overview: 58+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-10 15:02 [PATCH 0/5] Some fixes and an improvement for using CTest on Windows Johannes Schindelin via GitGitGadget
2022-08-10 15:02 ` [PATCH 1/5] cmake: align CTest definition with Git's CI runs Johannes Schindelin via GitGitGadget
2022-08-10 17:48 ` Junio C Hamano
2022-08-16 10:11 ` Johannes Schindelin
2022-08-16 15:15 ` Junio C Hamano
2022-08-19 13:57 ` Johannes Schindelin
2022-08-11 11:18 ` Ævar Arnfjörð Bjarmason
2022-08-10 15:02 ` [PATCH 2/5] cmake: copy the merge tools for testing Johannes Schindelin via GitGitGadget
2022-08-10 15:02 ` [PATCH 3/5] tests: explicitly skip `chmod` calls on Windows Johannes Schindelin via GitGitGadget
2022-08-11 11:22 ` Ævar Arnfjörð Bjarmason
2022-08-22 10:19 ` Johannes Schindelin
2022-08-23 7:34 ` Johannes Schindelin
2022-08-10 15:02 ` [PATCH 4/5] add -p: avoid ambiguous signed/unsigned comparison Johannes Schindelin via GitGitGadget
2022-08-10 17:54 ` Junio C Hamano
2022-08-16 9:56 ` Johannes Schindelin
2022-08-16 15:10 ` Junio C Hamano
2022-08-19 14:52 ` Johannes Schindelin
2022-08-11 12:49 ` Phillip Wood
2022-08-16 10:00 ` Johannes Schindelin
2022-08-16 14:23 ` Phillip Wood
2022-08-19 14:07 ` Johannes Schindelin
2022-08-10 15:02 ` [PATCH 5/5] cmake: avoid editing t/test-lib.sh Johannes Schindelin via GitGitGadget
2022-08-11 11:35 ` Ævar Arnfjörð Bjarmason
2022-10-18 14:02 ` Johannes Schindelin
2022-08-11 12:58 ` Phillip Wood
2022-08-16 10:09 ` Johannes Schindelin
2022-08-16 14:27 ` Phillip Wood
2022-08-23 8:30 ` [PATCH v2 0/5] Some fixes and an improvement for using CTest on Windows Johannes Schindelin via GitGitGadget
2022-08-23 8:30 ` [PATCH v2 1/5] cmake: make it easier to diagnose regressions in CTest runs Johannes Schindelin via GitGitGadget
2022-09-07 22:10 ` Victoria Dye
2022-10-18 14:02 ` Johannes Schindelin [this message]
2022-09-08 7:22 ` Ævar Arnfjörð Bjarmason
2022-09-28 6:55 ` Eric Sunshine
2022-08-23 8:31 ` [PATCH v2 2/5] cmake: copy the merge tools for testing Johannes Schindelin via GitGitGadget
2022-08-23 8:31 ` [PATCH v2 3/5] add -p: avoid ambiguous signed/unsigned comparison Johannes Schindelin via GitGitGadget
2022-08-23 8:31 ` [PATCH v2 4/5] cmake: avoid editing t/test-lib.sh Johannes Schindelin via GitGitGadget
2022-09-08 7:39 ` Ævar Arnfjörð Bjarmason
2022-10-18 14:03 ` Johannes Schindelin
2022-10-18 15:09 ` Ævar Arnfjörð Bjarmason
2022-09-08 23:37 ` Victoria Dye
2022-09-08 23:42 ` Victoria Dye
2022-09-08 23:58 ` Junio C Hamano
2022-10-18 14:03 ` Johannes Schindelin
2022-08-23 8:31 ` [PATCH v2 5/5] cmake: increase time-out for a long-running test Johannes Schindelin via GitGitGadget
2022-09-08 7:34 ` Ævar Arnfjörð Bjarmason
2022-09-08 17:29 ` Victoria Dye
2022-09-08 3:51 ` [PATCH v2 0/5] Some fixes and an improvement for using CTest on Windows Victoria Dye
2022-10-18 10:59 ` [PATCH v3 " Johannes Schindelin via GitGitGadget
2022-10-18 10:59 ` [PATCH v3 1/5] cmake: make it easier to diagnose regressions in CTest runs Johannes Schindelin via GitGitGadget
2022-10-18 13:41 ` Ævar Arnfjörð Bjarmason
2022-10-18 10:59 ` [PATCH v3 2/5] cmake: copy the merge tools for testing Johannes Schindelin via GitGitGadget
2022-10-18 13:49 ` Ævar Arnfjörð Bjarmason
2022-10-18 10:59 ` [PATCH v3 3/5] add -p: avoid ambiguous signed/unsigned comparison Johannes Schindelin via GitGitGadget
2022-10-18 13:53 ` Ævar Arnfjörð Bjarmason
2022-10-18 10:59 ` [PATCH v3 4/5] cmake: avoid editing t/test-lib.sh Johannes Schindelin via GitGitGadget
2022-10-18 13:54 ` Ævar Arnfjörð Bjarmason
2022-10-18 14:21 ` Johannes Schindelin
2022-10-18 10:59 ` [PATCH v3 5/5] cmake: increase time-out for a long-running test Johannes Schindelin via GitGitGadget
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=29r609np-s0r1-r904-16o3-6918930o2ror@tzk.qr \
--to=johannes.schindelin@gmx.de \
--cc=avarab@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=phillip.wood123@gmail.com \
--cc=vdye@github.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).