Re: [PATCH] tests: Disable migration-test

[Peter Xu]

On Tue, Feb 21, 2023 at 04:09:42PM +0000, Daniel P. Berrangé wrote:
> On Tue, Feb 21, 2023 at 03:36:39PM +0000, Dr. David Alan Gilbert wrote:
> > * Peter Maydell (peter.maydell@linaro.org) wrote:
> > > On Tue, 21 Feb 2023 at 15:21, Dr. David Alan Gilbert
> > > <dgilbert@redhat.com> wrote:
> > > > Damn this is really going to impact the stability of migration if we
> > > > don't regularly test.
> > > > But fundamentally, I've never been able to debug much of the reports
> > > > that come from flakyness in gitlab ci; we're not getting the most basic
> > > > information like which subtest or where we're upto in the test which
> > > > makes it very very hard to debug.
> > > 
> > > Right, but if you want more information you need to change the
> > > tests and/or test harness to provide it.
> > 
> > I don't think the migration test is doing anything odd in that respect;
> > We've just got a bunch of qtest tests; having a test framework which
> > doesn't tell you which test failed is very difficult.
> 
> Right so the problem here is the use of 'bail out'. From the POV
> of the TAP output format that is an immediate termination, so there
> is nothing to report about which test was being run.
> 
> To make the failing test visible, it needs to NOT trigger a bail
> out, but instead report an "not ok" line, which will show the
> test case name. AFAIK, this is a limitation of glib's test harness
> and its adoption of TAP. You can't get the test case name printed
> until the test case is finished. And glib tests fail by calling
> assert, so they will inherantly trigger 'bail out' logic from a
> TAP POV.
> 
> IIRC, the historical non-TAP output format would output the test
> case name first, and then once done report ok/not ok.
> 
> The workaround would be for glib to use a TAP diagnostic line
> to print the test case it is about to run. It already uses the
> diagnostic lines to report test groups eg
> 
> $ ./build//tests/unit/test-io-channel-command
> # random seed: R02S0718b3006d3dcf15099db36b61dff3e8
> 1..4
> # Start of io tests
> # Start of channel tests
> # Start of command tests
> # Start of fifo tests
> ok 1 /io/channel/command/fifo/sync
> ok 2 /io/channel/command/fifo/async
> # End of fifo tests
> # Start of echo tests
> **
> ERROR:../tests/unit/test-io-channel-command.c:102:test_io_channel_command_echo:
>  assertion failed: (rand() < 0.5)
> Bail out! 
> ERROR:../tests/unit/test-io-channel-command.c:102:test_io_channel_command_echo:
>  assertion failed: (rand() < 0.5)
> Aborted (core dumped)
> 
> 
> would have to be changed to report
> 
> $ ./build//tests/unit/test-io-channel-command
> # random seed: R02S0718b3006d3dcf15099db36b61dff3e8
> 1..4
> # Start of io tests
> # Start of channel tests
> # Start of command tests
> # Start of fifo tests
> # Running /io/channel/command/fifo/sync
> ok 1 /io/channel/command/fifo/sync
> # Running /io/channel/command/fifo/async
> ok 2 /io/channel/command/fifo/async
> # End of fifo tests
> # Start of echo tests
> # Running /io/channel/command/echo/sync
> **
> ERROR:../tests/unit/test-io-channel-command.c:102:test_io_channel_command_echo:
>  assertion failed: (rand() < 0.5)
> Bail out! 
> ERROR:../tests/unit/test-io-channel-command.c:102:test_io_channel_command_echo:
>  assertion failed: (rand() < 0.5)
> Aborted (core dumped)
> 
> so we see exactly what was running.
> 
> Without this though, we can still figure out what was running. You
> have to look back for the previous 'ok' line and see what it was.
> Then locally run '/path/to/test -l' to list the test case names.
> The one you want is the one immediately after the last 'ok' (not ok)
> line.
> 
> Rather tedious for sure, but not impossible.
> 
> Worth an RFE for glib, but would be a while before we saw the benefit.
> 
> As a workaround, we could print out TAP diagnostic output ourselves
> in any qtests that we see as especially unreliable. A diagnostic
> output is any line printed on stdout that starts with a '# '

The other possible way is I'm wondering whether we can try to do a whole
stack dump when an assertion happened.

With a backtrace we can easily see the function pointer so that'll be
another way of knowing which test we're running, slightly more awkward than
dumping the names but probably still good enough for developers.

It also contains more information so sometimes we can already spot the bug
even before trying to reproduce.

Though I'm not sure whether it's easily feasible, e.g., the tests are
mostly using g_assert*() families and I've no idea of any good way to let
it happen if without replacing all of them to inject a dump.

Thanks,

-- 
Peter Xu