Thomas Huth <th...@redhat.com> wrote:
> On 28/09/2023 15.32, Fabiano Rosas wrote:
>> Thomas Huth <th...@redhat.com> writes:
>>
>>> On 27/09/2023 23.47, Fabiano Rosas wrote:
>>>> Add a smoke test that migrates to a file and gives it to the
>>>> script. It should catch the most annoying errors such as changes in
>>>> the ram flags.
>>>>
>>>> After code has been merged it becomes way harder to figure out what is
>>>> causing the script to fail, the person making the change is the most
>>>> likely to know right away what the problem is.
>>>>
>>>> Signed-off-by: Fabiano Rosas <faro...@suse.de>
>>>> ---
>>>> I know this adds a python dependency to qtests and I'm not sure how
>>>> much we care about this script, but on the other hand it would be nice
>>>> to catch these errors early on.
>>>>
>>>> This would also help with future work that touches the migration
>>>> stream (moving multifd out of ram.c and fixed-ram).
>>>>
>>>> Let me know what you think.
>>>
>>> Without looking at this too closely, my first thought was: This sounds
>>> rather like a good candidate for an avocado test instead. It's using Python,
>>> so tests/avocado/ sounds like a better fit. Have you considered adding it as
>>> an avocado test already?
>>>
>> I intended to keep all migration tests at the same place. And well,
>> to
>> be honest, I have given up on avocado. Too unmaintained, incrutable
>> logging and last time I tried to use it locally, it was leaving stale
>> processes behind upon failure.
>> Of course, if that's the preferred place to put python tests I could
>> do
>> it, but I don't find it too compelling.
>
> Well, I guess this test here is kind of borderline, since it does not
> introduce new Python code, but just calls a pre-existing python
> script...
> maybe that's still ok for the qtests ... what do other people think?
I agree with adding it to migration-tests.
We can create a different test if necessary.
Reason for that is that this script is very useful (not enough) to find
interversion compatibilities.
We never found that it is failing after the release is done, So testing
it continously is much better in my humble opinion.
>>> >+#define ANALYZE_SCRIPT "tests/qtest/analyze-migration.py"
>>>
>>> Why can't you use scripts/analyze-migration.py directly?
>>>
>> I'm not entirely sure that's the case with QEMU, but generally build
>> directories can move/not be directly under the source tree. The test
>> wouldn't know from where to fetch the script.
>
> AFAIK the build system puts a symlink for the scripts folder into the
> build directory, at least I have one in mine.
>
>>> >+ file = g_strdup_printf("%s/migfile", tmpfs);
>>>
>>> Please, no static file names for temporary files - tests might be running in
>>> parallel, and then you get race conditions! Use something like
>>> g_file_open_tmp() instead to create a file with a random name.
>>>
>> Ok, I can do that. However, if you look for "tmpfs" in
>> migration-test.c
>> you'll see that's done all over the place. I'm thinking individual tests
>> under glib are never run in parallel. At least for the migration suite.
>
> Ah, sorry, my bad, I thought that tmpfs was simply pointing to "/tmp",
> but in fact it already contains a randomized name. So never mind, I
> think it should be fine what you're doing here.
I am changing that in a series that I have to rebase.
Stay tuned.
Later, Juan.
