| Message ID | 20231204163257.1011556-1-andrey.drobyshev@virtuozzo.com |
|---|---|
| State | New |
| Headers | show |
| Series | Revert "test/qga: use G_TEST_DIR to locate os-release test file" | expand |
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 > >
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 >> >> > >
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 > >> > >> > > > > >
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 >>>> >>>> >>> >>> >> > >
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);
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(-)