[PATCH v2 0/6] Final changes to fix test suite on macOS
The set of changes fixes qemuxml2argvtest and closes issue #58. Changes since v1: - Simplified lookup of symbols in main executable (Michal) - Reduced timeout to 90s for qemuxml2argvtest (Andrea) - Two new patches to fix qemucapsprobe Pipeline results: https://gitlab.com/roolebo/libvirt/-/pipelines/220225729 Roman Bolshakov (6): tests: Fix opendir mocks on macOS tests: Fix mock chaining on macOS qemuxml2argvtest: Increase timeout on macOS ci: Run test suite on macOS tests: Delay mock creation qemucapsprobemock: Fix lookup of qemu functions ci/cirrus/build.yml | 3 +-- tests/meson.build | 49 +++++++++++++++++++++++++----------------- tests/virfilewrapper.c | 4 ++++ tests/virmock.h | 13 +++++++++++ tests/virpcimock.c | 5 +++++ 5 files changed, 52 insertions(+), 22 deletions(-) -- 2.29.2
opendir() mocks need to search for decorated function with $INODE64 suffix, like stat mocks. Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com> --- tests/virfilewrapper.c | 4 ++++ tests/virpcimock.c | 4 ++++ 2 files changed, 8 insertions(+) diff --git a/tests/virfilewrapper.c b/tests/virfilewrapper.c index 0500a3617e..ca2356b5c9 100644 --- a/tests/virfilewrapper.c +++ b/tests/virfilewrapper.c @@ -56,7 +56,11 @@ static void init_syms(void) VIR_MOCK_REAL_INIT(access); VIR_MOCK_REAL_INIT(mkdir); VIR_MOCK_REAL_INIT(open); +# ifdef __APPLE__ + VIR_MOCK_REAL_INIT_ALIASED(opendir, "opendir$INODE64"); +# else VIR_MOCK_REAL_INIT(opendir); +# endif VIR_MOCK_REAL_INIT(execv); VIR_MOCK_REAL_INIT(execve); } diff --git a/tests/virpcimock.c b/tests/virpcimock.c index 5cd688c825..686f894e99 100644 --- a/tests/virpcimock.c +++ b/tests/virpcimock.c @@ -935,7 +935,11 @@ init_syms(void) VIR_MOCK_REAL_INIT(__open_2); # endif /* ! __GLIBC__ */ VIR_MOCK_REAL_INIT(close); +# ifdef __APPLE__ + VIR_MOCK_REAL_INIT_ALIASED(opendir, "opendir$INODE64"); +# else VIR_MOCK_REAL_INIT(opendir); +# endif VIR_MOCK_REAL_INIT(virFileCanonicalizePath); } -- 2.29.2
Some tests in qemuxml2argvtest need opendir() from virpcimock, others need opendir() from virfilewrapper. But as of now, only opendir() from virpcimock has an effect. real_opendir in virpcimock has a pointer to opendir$INODE64 in libsystem_kernel.dylib instead of pointing to opendir$INODE64 in qemuxml2argvtest (from virfilewrapper). And because the second one is never used, tests that rely on prefixes added by virFileWrapperAddPrefix fail. That can be fixed if dlsym(3) is asked explicitly to search symbols in main executable with RTLD_MAIN_ONLY before going to other dylibs. Existing RTLD_NEXT handle results into libsystem_kernel.dylib being searched before main executable. Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com> --- tests/virmock.h | 13 +++++++++++++ tests/virpcimock.c | 1 + 2 files changed, 14 insertions(+) diff --git a/tests/virmock.h b/tests/virmock.h index dea5feb80f..5250e733a1 100644 --- a/tests/virmock.h +++ b/tests/virmock.h @@ -284,9 +284,21 @@ static void (*real_##name)(void); \ void wrap_##name(void) +#if defined(VIR_MOCK_LOOKUP_MAIN) && defined(__APPLE__) +# define VIR_MOCK_REAL_INIT_MAIN(name, alias) \ + do { \ + if (real_##name == NULL) { \ + real_##name = dlsym(RTLD_MAIN_ONLY, alias); \ + } \ + } while (0) +#else +# define VIR_MOCK_REAL_INIT_MAIN(name, alias) \ + do {} while (0) +#endif #define VIR_MOCK_REAL_INIT(name) \ do { \ + VIR_MOCK_REAL_INIT_MAIN(name, #name); \ if (real_##name == NULL && \ !(real_##name = dlsym(RTLD_NEXT, \ #name))) { \ @@ -297,6 +309,7 @@ #define VIR_MOCK_REAL_INIT_ALIASED(name, alias) \ do { \ + VIR_MOCK_REAL_INIT_MAIN(name, alias); \ if (real_##name == NULL && \ !(real_##name = dlsym(RTLD_NEXT, \ alias))) { \ diff --git a/tests/virpcimock.c b/tests/virpcimock.c index 686f894e99..4aa96cae08 100644 --- a/tests/virpcimock.c +++ b/tests/virpcimock.c @@ -19,6 +19,7 @@ #include <config.h> #if defined(__linux__) || defined(__FreeBSD__) || defined(__APPLE__) +# define VIR_MOCK_LOOKUP_MAIN # include "virmock.h" # include <unistd.h> # include <fcntl.h> -- 2.29.2
On 11/23/20 11:10 PM, Roman Bolshakov wrote:
Some tests in qemuxml2argvtest need opendir() from virpcimock, others need opendir() from virfilewrapper.
But as of now, only opendir() from virpcimock has an effect. real_opendir in virpcimock has a pointer to opendir$INODE64 in libsystem_kernel.dylib instead of pointing to opendir$INODE64 in qemuxml2argvtest (from virfilewrapper). And because the second one is never used, tests that rely on prefixes added by virFileWrapperAddPrefix fail.
That can be fixed if dlsym(3) is asked explicitly to search symbols in main executable with RTLD_MAIN_ONLY before going to other dylibs. Existing RTLD_NEXT handle results into libsystem_kernel.dylib being searched before main executable.
Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com> --- tests/virmock.h | 13 +++++++++++++ tests/virpcimock.c | 1 + 2 files changed, 14 insertions(+)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
The test takes 40+ seconds on MBP 2012, MBA 2015. Cirrus completes the test within default timeout, just above 29 seconds but the error margin is narrow, under a second. It'd be good to provide reasonable default timeout to avoid test suite failure if "meson test" is invoked without arguments. Closes https://gitlab.com/libvirt/libvirt/-/issues/58 Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com> --- tests/meson.build | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/tests/meson.build b/tests/meson.build index f88410ff33..cb06873ba5 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -574,7 +574,13 @@ foreach data : tests ], export_dynamic: true, ) - test(data['name'], test_bin, env: tests_env) + if data['name'] == 'qemuxml2argvtest' and host_machine.system() == 'darwin' + timeout = 90 + else + # default meson timeout + timeout = 30 + endif + test(data['name'], test_bin, env: tests_env, timeout: timeout) endforeach -- 2.29.2
On 11/23/20 11:10 PM, Roman Bolshakov wrote:
The test takes 40+ seconds on MBP 2012, MBA 2015. Cirrus completes the test within default timeout, just above 29 seconds but the error margin is narrow, under a second.
It'd be good to provide reasonable default timeout to avoid test suite failure if "meson test" is invoked without arguments.
Closes https://gitlab.com/libvirt/libvirt/-/issues/58 Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com> --- tests/meson.build | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/tests/meson.build b/tests/meson.build index f88410ff33..cb06873ba5 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -574,7 +574,13 @@ foreach data : tests ], export_dynamic: true, ) - test(data['name'], test_bin, env: tests_env) + if data['name'] == 'qemuxml2argvtest' and host_machine.system() == 'darwin'
If we are doing this, then I'd say do it host arch agnostic.
+ timeout = 90 + else + # default meson timeout + timeout = 30 + endif + test(data['name'], test_bin, env: tests_env, timeout: timeout) endforeach
Michal
On Fri, Nov 27, 2020 at 10:53:49AM +0100, Michal Prívozník wrote:
On 11/23/20 11:10 PM, Roman Bolshakov wrote:
The test takes 40+ seconds on MBP 2012, MBA 2015. Cirrus completes the test within default timeout, just above 29 seconds but the error margin is narrow, under a second.
It'd be good to provide reasonable default timeout to avoid test suite failure if "meson test" is invoked without arguments.
Closes https://gitlab.com/libvirt/libvirt/-/issues/58 Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com> --- tests/meson.build | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/tests/meson.build b/tests/meson.build index f88410ff33..cb06873ba5 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -574,7 +574,13 @@ foreach data : tests ], export_dynamic: true, ) - test(data['name'], test_bin, env: tests_env) + if data['name'] == 'qemuxml2argvtest' and host_machine.system() == 'darwin'
If we are doing this, then I'd say do it host arch agnostic.
I intended to do that but forgot about it. Thanks for catching that! -Roman
+ timeout = 90 + else + # default meson timeout + timeout = 30 + endif + test(data['name'], test_bin, env: tests_env, timeout: timeout) endforeach
Michal
On 11/27/20 12:52 PM, Roman Bolshakov wrote:
On Fri, Nov 27, 2020 at 10:53:49AM +0100, Michal Prívozník wrote:
On 11/23/20 11:10 PM, Roman Bolshakov wrote:
The test takes 40+ seconds on MBP 2012, MBA 2015. Cirrus completes the test within default timeout, just above 29 seconds but the error margin is narrow, under a second.
It'd be good to provide reasonable default timeout to avoid test suite failure if "meson test" is invoked without arguments.
Closes https://gitlab.com/libvirt/libvirt/-/issues/58 Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com> --- tests/meson.build | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/tests/meson.build b/tests/meson.build index f88410ff33..cb06873ba5 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -574,7 +574,13 @@ foreach data : tests ], export_dynamic: true, ) - test(data['name'], test_bin, env: tests_env) + if data['name'] == 'qemuxml2argvtest' and host_machine.system() == 'darwin'
If we are doing this, then I'd say do it host arch agnostic.
I intended to do that but forgot about it. Thanks for catching that!
Okay, I can fix that just before pushing. Reviewed-by: Michal Privoznik <mprivozn@redhat.com> and pushed. Michal
On Fri, Nov 27, 2020 at 04:37:53PM +0100, Michal Prívozník wrote:
On 11/27/20 12:52 PM, Roman Bolshakov wrote:
On Fri, Nov 27, 2020 at 10:53:49AM +0100, Michal Prívozník wrote:
On 11/23/20 11:10 PM, Roman Bolshakov wrote:
The test takes 40+ seconds on MBP 2012, MBA 2015. Cirrus completes the test within default timeout, just above 29 seconds but the error margin is narrow, under a second.
It'd be good to provide reasonable default timeout to avoid test suite failure if "meson test" is invoked without arguments.
Closes https://gitlab.com/libvirt/libvirt/-/issues/58 Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com> --- tests/meson.build | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/tests/meson.build b/tests/meson.build index f88410ff33..cb06873ba5 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -574,7 +574,13 @@ foreach data : tests ], export_dynamic: true, ) - test(data['name'], test_bin, env: tests_env) + if data['name'] == 'qemuxml2argvtest' and host_machine.system() == 'darwin'
If we are doing this, then I'd say do it host arch agnostic.
I intended to do that but forgot about it. Thanks for catching that!
Okay, I can fix that just before pushing.
Reviewed-by: Michal Privoznik <mprivozn@redhat.com>
and pushed.
Michal
I was about to resend v3 when received the email :) Thanks for reviewing the series. Best regards, Roman
There's no need to have different CI process between macOS and FreeBSD as test suite has been fixed on macOS. Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com> --- ci/cirrus/build.yml | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ci/cirrus/build.yml b/ci/cirrus/build.yml index 912284b906..dc1cccd135 100644 --- a/ci/cirrus/build.yml +++ b/ci/cirrus/build.yml @@ -20,5 +20,4 @@ build_task: - git reset --hard "$CI_COMMIT_SHA" build_script: - meson build --prefix=$(pwd)/install-root - - if test "$(uname)" = "FreeBSD"; then ninja -C build dist; fi - - if test "$(uname)" = "Darwin"; then ninja -C build && ninja -C build install; fi + - ninja -C build dist -- 2.29.2
On 11/23/20 11:10 PM, Roman Bolshakov wrote:
There's no need to have different CI process between macOS and FreeBSD as test suite has been fixed on macOS.
Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com> --- ci/cirrus/build.yml | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
There might be mocks that need to reference qemu test driver and link with it. It's not possible now because qemu test driver is defined after mocks. While at it, add 'link_with' parameter to mock definition that allows to specify a set of libraries the mock has to be linked with. Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com> --- tests/meson.build | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/tests/meson.build b/tests/meson.build index cb06873ba5..ef8ded1ea4 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -115,24 +115,6 @@ if conf.has('WITH_SECDRIVER_SELINUX') ] endif -foreach mock : mock_libs - shared_library( - mock['name'], - mock.get('sources', [ '@0@.c'.format(mock['name']) ]), - override_options: [ - 'b_asneeded=false', - 'b_lundef=false', - ], - dependencies: [ - tests_dep, - mock.get('deps', []), - ], - link_with: [ - libvirt_lib, - ], - ) -endforeach - # build libraries used by tests @@ -201,6 +183,24 @@ test_file_wrapper_lib = static_library( dependencies: [ tests_dep ], ) +foreach mock : mock_libs + shared_library( + mock['name'], + mock.get('sources', [ '@0@.c'.format(mock['name']) ]), + override_options: [ + 'b_asneeded=false', + 'b_lundef=false', + ], + dependencies: [ + tests_dep, + mock.get('deps', []), + ], + link_with: [ + libvirt_lib, + mock.get('link_with', []), + ], + ) +endforeach # build helpers used by tests -- 2.29.2
On 11/23/20 11:10 PM, Roman Bolshakov wrote:
There might be mocks that need to reference qemu test driver and link with it. It's not possible now because qemu test driver is defined after mocks.
While at it, add 'link_with' parameter to mock definition that allows to specify a set of libraries the mock has to be linked with.
Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com> --- tests/meson.build | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Although I'd expect these foreach loops run only after the whole file was processed. Just like 'make' does. Michal
qemucapsprobemock can't find real versions of qemuMonitorSend() and qemuMonitorJSONIOProcessLine() on macOS. That breaks qemucapsprobe. The failure can be explained by documented behaviour of dlsym(3) on macOS: If dlsym() is called with the special handle RTLD_NEXT, then dyld searches for the symbol in the dylibs the calling image linked against when built. [...] For flat linked images, the search starts in the load ordered list of all images, in the image right after the caller's image. That means qemucapsprobemock must be linked against qemu test driver to find symbols there with RTLD_NEXT. Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com> --- tests/meson.build | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/meson.build b/tests/meson.build index ef8ded1ea4..cc0657f3c1 100644 --- a/tests/meson.build +++ b/tests/meson.build @@ -101,7 +101,6 @@ endif if conf.has('WITH_QEMU') mock_libs += [ { 'name': 'qemucaps2xmlmock' }, - { 'name': 'qemucapsprobemock' }, { 'name': 'qemucpumock' }, { 'name': 'qemuhotplugmock' }, { 'name': 'qemuxml2argvmock' }, @@ -171,6 +170,10 @@ if conf.has('WITH_QEMU') link_whole: [ qemu_driver_impl ], link_with: [ libvirt_lib ], ) + + mock_libs += [ + { 'name': 'qemucapsprobemock', 'link_with': [ test_qemu_driver_lib ] }, + ] else test_qemu_driver_lib = [] test_utils_qemu_lib = [] -- 2.29.2
On 11/23/20 11:10 PM, Roman Bolshakov wrote:
qemucapsprobemock can't find real versions of qemuMonitorSend() and qemuMonitorJSONIOProcessLine() on macOS. That breaks qemucapsprobe.
The failure can be explained by documented behaviour of dlsym(3) on macOS:
If dlsym() is called with the special handle RTLD_NEXT, then dyld searches for the symbol in the dylibs the calling image linked against when built.
[...] For flat linked images, the search starts in the load ordered list of all images, in the image right after the caller's image.
That means qemucapsprobemock must be linked against qemu test driver to find symbols there with RTLD_NEXT.
Signed-off-by: Roman Bolshakov <r.bolshakov@yadro.com> --- tests/meson.build | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Michal Prívozník -
Roman Bolshakov