[PATCH] Revert "test/qga: use G_TEST_DIR to locate os-release test file"

Andrey Drobyshev posted 1 patch 11 months, 4 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/qemu tags/patchew/20231204163257.1011556-1-andrey.drobyshev@virtuozzo.com
Maintainers: Michael Roth <michael.roth@amd.com>, Konstantin Kostiuk <kkostiuk@redhat.com>
tests/unit/test-qga.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
[PATCH] Revert "test/qga: use G_TEST_DIR to locate os-release test file"
Posted by Andrey Drobyshev 11 months, 4 weeks ago
Since the commit a85d09269b QGA_OS_RELEASE variable points to the path
relative to the build dir.  Then on qemu-ga startup this path can't be
found as qemu-ga cwd is somewhere else, which leads to the test failure:

  # ./tests/unit/test-qga -p /qga/guest-get-osinfo
  # random seed: R02S3a90c22d77ff1070fbd844f4959cf4a4
  # Start of qga tests
  **
  ERROR:../tests/unit/test-qga.c:906:test_qga_guest_get_osinfo: 'str' should not be NULL
  Bail out! ERROR:../tests/unit/test-qga.c:906:test_qga_guest_get_osinfo: 'str' should not be NULL

Let's obtain the absolute path again.

This reverts commit a85d09269bb1a7071d3ce0f2957e3ca9dba7c047.

Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
---
 tests/unit/test-qga.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/tests/unit/test-qga.c b/tests/unit/test-qga.c
index 671e83cb86..47cf5e30ec 100644
--- a/tests/unit/test-qga.c
+++ b/tests/unit/test-qga.c
@@ -1034,10 +1034,12 @@ static void test_qga_guest_get_osinfo(gconstpointer data)
     g_autoptr(QDict) ret = NULL;
     char *env[2];
     QDict *val;
+    g_autofree gchar *cwd = NULL;
 
+    cwd = g_get_current_dir();
     env[0] = g_strdup_printf(
-        "QGA_OS_RELEASE=%s%c..%cdata%ctest-qga-os-release",
-        g_test_get_dir(G_TEST_DIST), G_DIR_SEPARATOR, G_DIR_SEPARATOR, G_DIR_SEPARATOR);
+        "QGA_OS_RELEASE=%s%ctests%cdata%ctest-qga-os-release",
+        cwd, G_DIR_SEPARATOR, G_DIR_SEPARATOR, G_DIR_SEPARATOR);
     env[1] = NULL;
     fixture_setup(&fixture, NULL, env);
 
-- 
2.39.3
Re: [PATCH] Revert "test/qga: use G_TEST_DIR to locate os-release test file"
Posted by Marc-André Lureau 11 months, 4 weeks ago
Hi

On Mon, Dec 4, 2023 at 8:33 PM Andrey Drobyshev
<andrey.drobyshev@virtuozzo.com> wrote:
>
> Since the commit a85d09269b QGA_OS_RELEASE variable points to the path
> relative to the build dir.  Then on qemu-ga startup this path can't be
> found as qemu-ga cwd is somewhere else, which leads to the test failure:
>
>   # ./tests/unit/test-qga -p /qga/guest-get-osinfo
>   # random seed: R02S3a90c22d77ff1070fbd844f4959cf4a4
>   # Start of qga tests
>   **
>   ERROR:../tests/unit/test-qga.c:906:test_qga_guest_get_osinfo: 'str' should not be NULL
>   Bail out! ERROR:../tests/unit/test-qga.c:906:test_qga_guest_get_osinfo: 'str' should not be NULL
>
> Let's obtain the absolute path again.

Can you detail how the build and the test is done?

If I recall correctly, this change was done in order to move qga to a
subproject(), but isn't strictly required at this point. Although I
believe it is more correct to lookup test data relative to
G_TEST_DIST.

>
> This reverts commit a85d09269bb1a7071d3ce0f2957e3ca9dba7c047.
>
> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> ---
>  tests/unit/test-qga.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/tests/unit/test-qga.c b/tests/unit/test-qga.c
> index 671e83cb86..47cf5e30ec 100644
> --- a/tests/unit/test-qga.c
> +++ b/tests/unit/test-qga.c
> @@ -1034,10 +1034,12 @@ static void test_qga_guest_get_osinfo(gconstpointer data)
>      g_autoptr(QDict) ret = NULL;
>      char *env[2];
>      QDict *val;
> +    g_autofree gchar *cwd = NULL;
>
> +    cwd = g_get_current_dir();
>      env[0] = g_strdup_printf(
> -        "QGA_OS_RELEASE=%s%c..%cdata%ctest-qga-os-release",
> -        g_test_get_dir(G_TEST_DIST), G_DIR_SEPARATOR, G_DIR_SEPARATOR, G_DIR_SEPARATOR);
> +        "QGA_OS_RELEASE=%s%ctests%cdata%ctest-qga-os-release",
> +        cwd, G_DIR_SEPARATOR, G_DIR_SEPARATOR, G_DIR_SEPARATOR);
>      env[1] = NULL;
>      fixture_setup(&fixture, NULL, env);
>
> --
> 2.39.3
>
>


-- 
Marc-André Lureau
Re: [PATCH] Revert "test/qga: use G_TEST_DIR to locate os-release test file"
Posted by Andrey Drobyshev 11 months, 4 weeks ago
On 12/4/23 18:51, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Dec 4, 2023 at 8:33 PM Andrey Drobyshev
> <andrey.drobyshev@virtuozzo.com> wrote:
>>
>> Since the commit a85d09269b QGA_OS_RELEASE variable points to the path
>> relative to the build dir.  Then on qemu-ga startup this path can't be
>> found as qemu-ga cwd is somewhere else, which leads to the test failure:
>>
>>   # ./tests/unit/test-qga -p /qga/guest-get-osinfo
>>   # random seed: R02S3a90c22d77ff1070fbd844f4959cf4a4
>>   # Start of qga tests
>>   **
>>   ERROR:../tests/unit/test-qga.c:906:test_qga_guest_get_osinfo: 'str' should not be NULL
>>   Bail out! ERROR:../tests/unit/test-qga.c:906:test_qga_guest_get_osinfo: 'str' should not be NULL
>>
>> Let's obtain the absolute path again.
> 
> Can you detail how the build and the test is done?
> 

Simple as:

> ./configure --cc=gcc --target-list=x86_64-softmmu --enable-guest-agent && make -j16
> cd build; tests/unit/test-qga -p /qga/guest-get-osinfo


> If I recall correctly, this change was done in order to move qga to a
> subproject(), but isn't strictly required at this point. Although I
> believe it is more correct to lookup test data relative to
> G_TEST_DIST.
> 

Then we'd have to change cwd of qemu-ga at startup to ensure relative
paths work.  Right now (with the initial change) it appears broken.

>>
>> This reverts commit a85d09269bb1a7071d3ce0f2957e3ca9dba7c047.
>>
>> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
>> ---
>>  tests/unit/test-qga.c | 6 ++++--
>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/unit/test-qga.c b/tests/unit/test-qga.c
>> index 671e83cb86..47cf5e30ec 100644
>> --- a/tests/unit/test-qga.c
>> +++ b/tests/unit/test-qga.c
>> @@ -1034,10 +1034,12 @@ static void test_qga_guest_get_osinfo(gconstpointer data)
>>      g_autoptr(QDict) ret = NULL;
>>      char *env[2];
>>      QDict *val;
>> +    g_autofree gchar *cwd = NULL;
>>
>> +    cwd = g_get_current_dir();
>>      env[0] = g_strdup_printf(
>> -        "QGA_OS_RELEASE=%s%c..%cdata%ctest-qga-os-release",
>> -        g_test_get_dir(G_TEST_DIST), G_DIR_SEPARATOR, G_DIR_SEPARATOR, G_DIR_SEPARATOR);
>> +        "QGA_OS_RELEASE=%s%ctests%cdata%ctest-qga-os-release",
>> +        cwd, G_DIR_SEPARATOR, G_DIR_SEPARATOR, G_DIR_SEPARATOR);
>>      env[1] = NULL;
>>      fixture_setup(&fixture, NULL, env);
>>
>> --
>> 2.39.3
>>
>>
> 
> 


Re: [PATCH] Revert "test/qga: use G_TEST_DIR to locate os-release test file"
Posted by Marc-André Lureau 11 months, 4 weeks ago
Hi

On Mon, Dec 4, 2023 at 9:01 PM Andrey Drobyshev
<andrey.drobyshev@virtuozzo.com> wrote:
>
> On 12/4/23 18:51, Marc-André Lureau wrote:
> > Hi
> >
> > On Mon, Dec 4, 2023 at 8:33 PM Andrey Drobyshev
> > <andrey.drobyshev@virtuozzo.com> wrote:
> >>
> >> Since the commit a85d09269b QGA_OS_RELEASE variable points to the path
> >> relative to the build dir.  Then on qemu-ga startup this path can't be
> >> found as qemu-ga cwd is somewhere else, which leads to the test failure:
> >>
> >>   # ./tests/unit/test-qga -p /qga/guest-get-osinfo
> >>   # random seed: R02S3a90c22d77ff1070fbd844f4959cf4a4
> >>   # Start of qga tests
> >>   **
> >>   ERROR:../tests/unit/test-qga.c:906:test_qga_guest_get_osinfo: 'str' should not be NULL
> >>   Bail out! ERROR:../tests/unit/test-qga.c:906:test_qga_guest_get_osinfo: 'str' should not be NULL
> >>
> >> Let's obtain the absolute path again.
> >
> > Can you detail how the build and the test is done?
> >
>
> Simple as:
>
> > ./configure --cc=gcc --target-list=x86_64-softmmu --enable-guest-agent && make -j16
> > cd build; tests/unit/test-qga -p /qga/guest-get-osinfo
>
>
> > If I recall correctly, this change was done in order to move qga to a
> > subproject(), but isn't strictly required at this point. Although I
> > believe it is more correct to lookup test data relative to
> > G_TEST_DIST.
> >
>
> Then we'd have to change cwd of qemu-ga at startup to ensure relative
> paths work.  Right now (with the initial change) it appears broken.

By reverting the patch, it is _still_ broken if you run the test
manually from a different directory (say from tests/unit for example)

With G_TEST_DIST, and proper testing environment, it works from any directory.

Tests are not meant to be run manually, you should run them through
the test runner: meson test -v test-qga

>
> >>
> >> This reverts commit a85d09269bb1a7071d3ce0f2957e3ca9dba7c047.
> >>
> >> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
> >> ---
> >>  tests/unit/test-qga.c | 6 ++++--
> >>  1 file changed, 4 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/tests/unit/test-qga.c b/tests/unit/test-qga.c
> >> index 671e83cb86..47cf5e30ec 100644
> >> --- a/tests/unit/test-qga.c
> >> +++ b/tests/unit/test-qga.c
> >> @@ -1034,10 +1034,12 @@ static void test_qga_guest_get_osinfo(gconstpointer data)
> >>      g_autoptr(QDict) ret = NULL;
> >>      char *env[2];
> >>      QDict *val;
> >> +    g_autofree gchar *cwd = NULL;
> >>
> >> +    cwd = g_get_current_dir();
> >>      env[0] = g_strdup_printf(
> >> -        "QGA_OS_RELEASE=%s%c..%cdata%ctest-qga-os-release",
> >> -        g_test_get_dir(G_TEST_DIST), G_DIR_SEPARATOR, G_DIR_SEPARATOR, G_DIR_SEPARATOR);
> >> +        "QGA_OS_RELEASE=%s%ctests%cdata%ctest-qga-os-release",
> >> +        cwd, G_DIR_SEPARATOR, G_DIR_SEPARATOR, G_DIR_SEPARATOR);
> >>      env[1] = NULL;
> >>      fixture_setup(&fixture, NULL, env);
> >>
> >> --
> >> 2.39.3
> >>
> >>
> >
> >
>


-- 
Marc-André Lureau
Re: [PATCH] Revert "test/qga: use G_TEST_DIR to locate os-release test file"
Posted by Andrey Drobyshev 11 months, 4 weeks ago
On 12/4/23 19:09, Marc-André Lureau wrote:
> Hi
> 
> On Mon, Dec 4, 2023 at 9:01 PM Andrey Drobyshev
> <andrey.drobyshev@virtuozzo.com> wrote:
>>
>> On 12/4/23 18:51, Marc-André Lureau wrote:
>>> Hi
>>>
>>> On Mon, Dec 4, 2023 at 8:33 PM Andrey Drobyshev
>>> <andrey.drobyshev@virtuozzo.com> wrote:
>>>>
>>>> Since the commit a85d09269b QGA_OS_RELEASE variable points to the path
>>>> relative to the build dir.  Then on qemu-ga startup this path can't be
>>>> found as qemu-ga cwd is somewhere else, which leads to the test failure:
>>>>
>>>>   # ./tests/unit/test-qga -p /qga/guest-get-osinfo
>>>>   # random seed: R02S3a90c22d77ff1070fbd844f4959cf4a4
>>>>   # Start of qga tests
>>>>   **
>>>>   ERROR:../tests/unit/test-qga.c:906:test_qga_guest_get_osinfo: 'str' should not be NULL
>>>>   Bail out! ERROR:../tests/unit/test-qga.c:906:test_qga_guest_get_osinfo: 'str' should not be NULL
>>>>
>>>> Let's obtain the absolute path again.
>>>
>>> Can you detail how the build and the test is done?
>>>
>>
>> Simple as:
>>
>>> ./configure --cc=gcc --target-list=x86_64-softmmu --enable-guest-agent && make -j16
>>> cd build; tests/unit/test-qga -p /qga/guest-get-osinfo
>>
>>
>>> If I recall correctly, this change was done in order to move qga to a
>>> subproject(), but isn't strictly required at this point. Although I
>>> believe it is more correct to lookup test data relative to
>>> G_TEST_DIST.
>>>
>>
>> Then we'd have to change cwd of qemu-ga at startup to ensure relative
>> paths work.  Right now (with the initial change) it appears broken.
> 
> By reverting the patch, it is _still_ broken if you run the test
> manually from a different directory (say from tests/unit for example)
>
> With G_TEST_DIST, and proper testing environment, it works from any directory.
> 

No, it seems to be failing as well, only earlier.  Before the revert:
> cd build/tests/unit; ./test-qga 
> # random seed: R02S450ef942c699b5af6dff48f9c5b73b33
> **
> ERROR:../tests/unit/test-qga.c:79:fixture_setup: assertion failed (error == NULL): Failed to execute child process “$SRC/build/tests/unit/qga/qemu-ga” (No such file or directory) (g-exec-error-quark, 8)
> Bail out! ERROR:../tests/unit/test-qga.c:79:fixture_setup: assertion failed (error == NULL): Failed to execute child process “$SRC/build/tests/unit/qga/qemu-ga” (No such file or directory) (g-exec-error-quark, 8)

But maybe my testing environment isn't proper?

> Tests are not meant to be run manually, you should run them through
> the test runner: meson test -v test-qga
> 

That's a good point, but I just found it suspicious that this is
literally the *only* case of the *only* unit test which fails (when run
directly from ./build).  Could we fix the direct execution as well then?

Btw test runner also cannot be run from just any directory, otherwise it
complains:
> meson test -v test-qga         
> 
> ERROR: No such build data file as '$SRC/build/tests/unit/meson-private/build.dat'.


>>
>>>>
>>>> This reverts commit a85d09269bb1a7071d3ce0f2957e3ca9dba7c047.
>>>>
>>>> Signed-off-by: Andrey Drobyshev <andrey.drobyshev@virtuozzo.com>
>>>> ---
>>>>  tests/unit/test-qga.c | 6 ++++--
>>>>  1 file changed, 4 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/tests/unit/test-qga.c b/tests/unit/test-qga.c
>>>> index 671e83cb86..47cf5e30ec 100644
>>>> --- a/tests/unit/test-qga.c
>>>> +++ b/tests/unit/test-qga.c
>>>> @@ -1034,10 +1034,12 @@ static void test_qga_guest_get_osinfo(gconstpointer data)
>>>>      g_autoptr(QDict) ret = NULL;
>>>>      char *env[2];
>>>>      QDict *val;
>>>> +    g_autofree gchar *cwd = NULL;
>>>>
>>>> +    cwd = g_get_current_dir();
>>>>      env[0] = g_strdup_printf(
>>>> -        "QGA_OS_RELEASE=%s%c..%cdata%ctest-qga-os-release",
>>>> -        g_test_get_dir(G_TEST_DIST), G_DIR_SEPARATOR, G_DIR_SEPARATOR, G_DIR_SEPARATOR);
>>>> +        "QGA_OS_RELEASE=%s%ctests%cdata%ctest-qga-os-release",
>>>> +        cwd, G_DIR_SEPARATOR, G_DIR_SEPARATOR, G_DIR_SEPARATOR);
>>>>      env[1] = NULL;
>>>>      fixture_setup(&fixture, NULL, env);
>>>>
>>>> --
>>>> 2.39.3
>>>>
>>>>
>>>
>>>
>>
> 
>