[libvirt] [PATCH] tests: Move tools under tests/tools/

Michal Privoznik posted 1 patch 11 weeks ago
Test syntax-check failed
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/3b1dcab2e1b1eb52cc408acbe8f5d40ad97ca043.1557129682.git.mprivozn@redhat.com
.gitignore                                  |  2 +-
Makefile.am                                 |  2 +-
cfg.mk                                      |  4 +-
configure.ac                                |  1 +
tests/Makefile.am                           | 22 +-----
tests/qemucapabilitiestest.c                |  4 +-
tests/testutils.c                           |  2 +-
tests/{ => tools}/.valgrind.supp            |  0
tests/tools/Makefile.am                     | 85 +++++++++++++++++++++
tests/{ => tools}/check-file-access.pl      |  0
tests/{ => tools}/file_access_whitelist.txt |  0
tests/{ => tools}/group-qemu-caps.pl        |  0
tests/{ => tools}/oomtrace.pl               |  0
tests/{ => tools}/qemucapsprobe.c           |  0
tests/{ => tools}/qemucapsprobemock.c       |  0
tests/{ => tools}/test-wrap-argv.pl         |  2 +-
16 files changed, 98 insertions(+), 26 deletions(-)
rename tests/{ => tools}/.valgrind.supp (100%)
create mode 100644 tests/tools/Makefile.am
rename tests/{ => tools}/check-file-access.pl (100%)
rename tests/{ => tools}/file_access_whitelist.txt (100%)
rename tests/{ => tools}/group-qemu-caps.pl (100%)
rename tests/{ => tools}/oomtrace.pl (100%)
rename tests/{ => tools}/qemucapsprobe.c (100%)
rename tests/{ => tools}/qemucapsprobemock.c (100%)
rename tests/{ => tools}/test-wrap-argv.pl (98%)

[libvirt] [PATCH] tests: Move tools under tests/tools/

Posted by Michal Privoznik 11 weeks ago
There are some scripts/binaries that are not tests themselves but
rather fulfill support purpose. Separate them from the rest of
the tests.

Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
---
 .gitignore                                  |  2 +-
 Makefile.am                                 |  2 +-
 cfg.mk                                      |  4 +-
 configure.ac                                |  1 +
 tests/Makefile.am                           | 22 +-----
 tests/qemucapabilitiestest.c                |  4 +-
 tests/testutils.c                           |  2 +-
 tests/{ => tools}/.valgrind.supp            |  0
 tests/tools/Makefile.am                     | 85 +++++++++++++++++++++
 tests/{ => tools}/check-file-access.pl      |  0
 tests/{ => tools}/file_access_whitelist.txt |  0
 tests/{ => tools}/group-qemu-caps.pl        |  0
 tests/{ => tools}/oomtrace.pl               |  0
 tests/{ => tools}/qemucapsprobe.c           |  0
 tests/{ => tools}/qemucapsprobemock.c       |  0
 tests/{ => tools}/test-wrap-argv.pl         |  2 +-
 16 files changed, 98 insertions(+), 26 deletions(-)
 rename tests/{ => tools}/.valgrind.supp (100%)
 create mode 100644 tests/tools/Makefile.am
 rename tests/{ => tools}/check-file-access.pl (100%)
 rename tests/{ => tools}/file_access_whitelist.txt (100%)
 rename tests/{ => tools}/group-qemu-caps.pl (100%)
 rename tests/{ => tools}/oomtrace.pl (100%)
 rename tests/{ => tools}/qemucapsprobe.c (100%)
 rename tests/{ => tools}/qemucapsprobemock.c (100%)
 rename tests/{ => tools}/test-wrap-argv.pl (98%)

diff --git a/.gitignore b/.gitignore
index 16eb4a3e2e..c231d394f3 100644
--- a/.gitignore
+++ b/.gitignore
@@ -170,7 +170,7 @@
 /tests/*.trs
 /tests/*test
 /tests/commandhelper
-/tests/qemucapsprobe
+/tests/tools/qemucapsprobe
 !/tests/virsh-self-test
 !/tests/virt-aa-helper-test
 !/tests/virt-admin-self-test
diff --git a/Makefile.am b/Makefile.am
index eba5916352..875c0fa997 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -17,7 +17,7 @@
 ## <http://www.gnu.org/licenses/>.
 
 SUBDIRS = . gnulib/lib include/libvirt src tools docs gnulib/tests \
-  tests po examples
+  tests tests/tools po examples
 
 XZ_OPT ?= -v -T0
 export XZ_OPT
diff --git a/cfg.mk b/cfg.mk
index b785089910..5e055023ee 100644
--- a/cfg.mk
+++ b/cfg.mk
@@ -1169,10 +1169,10 @@ header-ifdef:
 
 test-wrap-argv:
 	$(AM_V_GEN)$(VC_LIST) | $(GREP) -E '\.(ldargs|args)' | xargs \
-	$(PERL) $(top_srcdir)/tests/test-wrap-argv.pl --check
+	$(PERL) $(top_srcdir)/tests/tools/test-wrap-argv.pl --check
 
 group-qemu-caps:
-	$(AM_V_GEN)$(PERL) $(top_srcdir)/tests/group-qemu-caps.pl --check $(top_srcdir)/
+	$(AM_V_GEN)$(PERL) $(top_srcdir)/tests/tools/group-qemu-caps.pl --check $(top_srcdir)/
 
 # sc_po_check can fail if generated files are not built first
 sc_po_check: \
diff --git a/configure.ac b/configure.ac
index fabec815db..893d0db17a 100644
--- a/configure.ac
+++ b/configure.ac
@@ -925,6 +925,7 @@ AC_CONFIG_FILES([\
         include/libvirt/libvirt-common.h \
         examples/Makefile \
         tests/Makefile \
+        tests/tools/Makefile \
         tools/Makefile])
 AC_OUTPUT
 
diff --git a/tests/Makefile.am b/tests/Makefile.am
index 46d94d2236..0f5a5c231e 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -75,7 +75,6 @@ MOCKLIBS_LIBS = \
 	../src/libvirt.la
 
 EXTRA_DIST = \
-	.valgrind.supp \
 	bhyvexml2argvdata \
 	bhyveargv2xmldata \
 	bhyvexml2xmloutdata \
@@ -107,7 +106,6 @@ EXTRA_DIST = \
 	nwfilterxml2firewalldata \
 	nwfilterxml2xmlin \
 	nwfilterxml2xmlout \
-	oomtrace.pl \
 	qemuagentdata \
 	qemuargv2xmldata \
 	qemublocktestdata \
@@ -285,12 +283,10 @@ test_programs += qemuxml2argvtest qemuxml2xmltest \
 	qemusecuritytest \
 	qemufirmwaretest \
 	$(NULL)
-test_helpers += qemucapsprobe
 test_libraries += libqemumonitortestutils.la \
 		libqemutestdriver.la \
 		qemuxml2argvmock.la \
 		qemucaps2xmlmock.la \
-		qemucapsprobemock.la \
 		qemucpumock.la \
 		$(NULL)
 endif WITH_QEMU
@@ -442,15 +438,15 @@ EXTRA_DIST += $(test_scripts)
 if WITH_LINUX
 check-access: file-access-clean
 	VIR_TEST_FILE_ACCESS=1 $(MAKE) $(AM_MAKEFLAGS) check
-	$(PERL) check-file-access.pl | sort -u
+	$(PERL) tools/check-file-access.pl | sort -u
 
 file-access-clean:
 	> test_file_access.txt
 endif WITH_LINUX
 
 EXTRA_DIST += \
-	check-file-access.pl \
-	file_access_whitelist.txt
+	tools/check-file-access.pl \
+	tools/file_access_whitelist.txt
 
 if WITH_TESTS
 noinst_PROGRAMS = $(test_programs) $(test_helpers)
@@ -478,7 +474,7 @@ TESTS_ENVIRONMENT = \
 
 VALGRIND = valgrind --quiet --leak-check=full --trace-children=yes \
 	--trace-children-skip="*/tools/virsh","*/tests/commandhelper" \
-	--suppressions=$(abs_srcdir)/.valgrind.supp
+	--suppressions=$(abs_srcdir)/tools/.valgrind.supp
 valgrind:
 	$(MAKE) check VG="$(LIBTOOL) --mode=execute $(VALGRIND)"
 
@@ -603,16 +599,6 @@ qemucapabilitiestest_SOURCES = \
 qemucapabilitiestest_LDADD = libqemumonitortestutils.la \
 	$(qemu_LDADDS) $(LDADDS)
 
-qemucapsprobe_SOURCES = \
-	qemucapsprobe.c
-qemucapsprobe_LDADD = \
-	libqemutestdriver.la $(LDADDS)
-
-qemucapsprobemock_la_SOURCES = \
-	qemucapsprobemock.c
-qemucapsprobemock_la_LDFLAGS = $(MOCKLIBS_LDFLAGS)
-qemucapsprobemock_la_LIBADD = $(MOCKLIBS_LIBS)
-
 qemucommandutiltest_SOURCES = \
 	qemucommandutiltest.c \
 	testutils.c testutils.h \
diff --git a/tests/qemucapabilitiestest.c b/tests/qemucapabilitiestest.c
index ac9ab6bfce..48363326f4 100644
--- a/tests/qemucapabilitiestest.c
+++ b/tests/qemucapabilitiestest.c
@@ -225,11 +225,11 @@ mymain(void)
         return EXIT_FAILURE;
 
     /*
-     * Run "tests/qemucapsprobe /path/to/qemu/binary >foo.replies"
+     * Run "tests/tools/qemucapsprobe /path/to/qemu/binary >foo.replies"
      * to generate updated or new *.replies data files.
      *
      * If you manually edit replies files you can run
-     * "tests/qemucapsfixreplies foo.replies" to fix the replies ids.
+     * "tests/tools/qemucapsfixreplies foo.replies" to fix the replies ids.
      *
      * Once a replies file has been generated and tweaked if necessary,
      * you can drop it into tests/qemucapabilitiesdata/ (with a sensible
diff --git a/tests/testutils.c b/tests/testutils.c
index 245b1832f6..080a1ccda2 100644
--- a/tests/testutils.c
+++ b/tests/testutils.c
@@ -533,7 +533,7 @@ virTestRewrapFile(const char *filename)
         return -1;
     }
 
-    if (virAsprintf(&script, "%s/test-wrap-argv.pl", abs_srcdir) < 0)
+    if (virAsprintf(&script, "%s/tools/test-wrap-argv.pl", abs_srcdir) < 0)
         goto cleanup;
 
     cmd = virCommandNewArgList(perl, script, "--in-place", filename, NULL);
diff --git a/tests/.valgrind.supp b/tests/tools/.valgrind.supp
similarity index 100%
rename from tests/.valgrind.supp
rename to tests/tools/.valgrind.supp
diff --git a/tests/tools/Makefile.am b/tests/tools/Makefile.am
new file mode 100644
index 0000000000..8a34b4a84f
--- /dev/null
+++ b/tests/tools/Makefile.am
@@ -0,0 +1,85 @@
+# vim: filetype=automake
+
+AM_CPPFLAGS = \
+	-I$(top_srcdir)/tests/ \
+	-I$(top_builddir) -I$(top_srcdir) \
+	-I$(top_builddir)/gnulib/lib -I$(top_srcdir)/gnulib/lib \
+	-I$(top_builddir)/include -I$(top_srcdir)/include \
+	-I$(top_builddir)/src -I$(top_srcdir)/src \
+	-I$(top_srcdir)/src/util \
+	-I$(top_srcdir)/src/conf \
+	$(NULL)
+
+WARN_CFLAGS += $(RELAXED_FRAME_LIMIT_CFLAGS)
+
+AM_CFLAGS = \
+	-Dabs_builddir="\"$(abs_builddir)\"" \
+	-Dabs_top_builddir="\"$(abs_top_builddir)\"" \
+	-Dabs_srcdir="\"$(abs_srcdir)\"" \
+	-Dabs_top_srcdir="\"$(abs_top_srcdir)\"" \
+	$(LIBXML_CFLAGS) \
+	$(LIBNL_CFLAGS) \
+	$(GNUTLS_CFLAGS) \
+	$(SASL_CFLAGS) \
+	$(SELINUX_CFLAGS) \
+	$(APPARMOR_CFLAGS) \
+	$(YAJL_CFLAGS) \
+	$(XDR_CFLAGS) \
+	$(WARN_CFLAGS)
+
+AM_LDFLAGS = \
+	-export-dynamic
+
+MOCKLIBS_LDFLAGS = -module -avoid-version \
+	-rpath /evil/libtool/hack/to/force/shared/lib/creation \
+	$(MINGW_EXTRA_LDFLAGS)
+
+GNULIB_LIBS = \
+	../../gnulib/lib/libgnu.la
+
+MOCKLIBS_LIBS = \
+	$(GNULIB_LIBS) \
+	../../src/libvirt.la
+
+PROBES_O =
+if WITH_DTRACE_PROBES
+PROBES_O += ../../src/libvirt_probes.lo
+endif WITH_DTRACE_PROBES
+
+LDADDS = \
+	$(NO_INDIRECT_LDFLAGS) \
+	$(PROBES_O) \
+	$(GNULIB_LIBS) \
+	../../src/libvirt.la
+
+test_helpers =
+test_libraries =
+
+if WITH_QEMU
+test_helpers += qemucapsprobe
+test_libraries += qemucapsprobemock.la
+
+qemucapsprobe_SOURCES = \
+	qemucapsprobe.c \
+	../testutils.h
+qemucapsprobe_LDADD = \
+	../libqemutestdriver.la $(LDADDS)
+
+qemucapsprobemock_la_SOURCES = \
+	qemucapsprobemock.c
+qemucapsprobemock_la_LDFLAGS = $(MOCKLIBS_LDFLAGS)
+qemucapsprobemock_la_LIBADD = $(MOCKLIBS_LIBS)
+endif WITH_QEMU
+
+
+if WITH_TESTS
+noinst_PROGRAMS = $(test_helpers)
+noinst_LTLIBRARIES = $(test_libraries)
+else ! WITH_TESTS
+check_PROGRAMS = $(test_helpers)
+check_LTLIBRARIES = $(test_libraries)
+endif ! WITH_TESTS
+
+EXTRA_DIST = \
+	.valgrind.supp \
+	oomtrace.pl
diff --git a/tests/check-file-access.pl b/tests/tools/check-file-access.pl
similarity index 100%
rename from tests/check-file-access.pl
rename to tests/tools/check-file-access.pl
diff --git a/tests/file_access_whitelist.txt b/tests/tools/file_access_whitelist.txt
similarity index 100%
rename from tests/file_access_whitelist.txt
rename to tests/tools/file_access_whitelist.txt
diff --git a/tests/group-qemu-caps.pl b/tests/tools/group-qemu-caps.pl
similarity index 100%
rename from tests/group-qemu-caps.pl
rename to tests/tools/group-qemu-caps.pl
diff --git a/tests/oomtrace.pl b/tests/tools/oomtrace.pl
similarity index 100%
rename from tests/oomtrace.pl
rename to tests/tools/oomtrace.pl
diff --git a/tests/qemucapsprobe.c b/tests/tools/qemucapsprobe.c
similarity index 100%
rename from tests/qemucapsprobe.c
rename to tests/tools/qemucapsprobe.c
diff --git a/tests/qemucapsprobemock.c b/tests/tools/qemucapsprobemock.c
similarity index 100%
rename from tests/qemucapsprobemock.c
rename to tests/tools/qemucapsprobemock.c
diff --git a/tests/test-wrap-argv.pl b/tests/tools/test-wrap-argv.pl
similarity index 98%
rename from tests/test-wrap-argv.pl
rename to tests/tools/test-wrap-argv.pl
index 7867e9d719..4a28ee9d46 100755
--- a/tests/test-wrap-argv.pl
+++ b/tests/tools/test-wrap-argv.pl
@@ -94,7 +94,7 @@ sub rewrap {
             close DIFF;
 
             print STDERR "Incorrect line wrapping in $file\n";
-            print STDERR "Use test-wrap-argv.pl to wrap test data files\n";
+            print STDERR "Use tests/tools/test-wrap-argv.pl to wrap test data files\n";
             return -1;
         }
     } else {
-- 
2.21.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] tests: Move tools under tests/tools/

Posted by Daniel P. Berrangé 11 weeks ago
On Mon, May 06, 2019 at 10:01:37AM +0200, Michal Privoznik wrote:
> There are some scripts/binaries that are not tests themselves but
> rather fulfill support purpose. Separate them from the rest of
> the tests.
> 
> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> ---
>  .gitignore                                  |  2 +-
>  Makefile.am                                 |  2 +-
>  cfg.mk                                      |  4 +-
>  configure.ac                                |  1 +
>  tests/Makefile.am                           | 22 +-----
>  tests/qemucapabilitiestest.c                |  4 +-
>  tests/testutils.c                           |  2 +-
>  tests/{ => tools}/.valgrind.supp            |  0
>  tests/tools/Makefile.am                     | 85 +++++++++++++++++++++
>  tests/{ => tools}/check-file-access.pl      |  0
>  tests/{ => tools}/file_access_whitelist.txt |  0
>  tests/{ => tools}/group-qemu-caps.pl        |  0
>  tests/{ => tools}/oomtrace.pl               |  0
>  tests/{ => tools}/qemucapsprobe.c           |  0
>  tests/{ => tools}/qemucapsprobemock.c       |  0
>  tests/{ => tools}/test-wrap-argv.pl         |  2 +-
>  16 files changed, 98 insertions(+), 26 deletions(-)
>  rename tests/{ => tools}/.valgrind.supp (100%)
>  create mode 100644 tests/tools/Makefile.am
>  rename tests/{ => tools}/check-file-access.pl (100%)
>  rename tests/{ => tools}/file_access_whitelist.txt (100%)
>  rename tests/{ => tools}/group-qemu-caps.pl (100%)
>  rename tests/{ => tools}/oomtrace.pl (100%)
>  rename tests/{ => tools}/qemucapsprobe.c (100%)
>  rename tests/{ => tools}/qemucapsprobemock.c (100%)
>  rename tests/{ => tools}/test-wrap-argv.pl (98%)

If we're going to re-arrange our tests, then I would really like to
see the goal be to remove the top level "tests/" directory entirely.

Instead move the tests under the src/ sub-directory that corresponds
to the code being tested. eg we should have src/util/virhashtest.c
alongside src/util/virhash.c  So instead of one giant test dir we
have everything distributed.

That way if you are running "make" in the src/ directory, you don't
need to change dir to do tests. It will also let us split up the
ever growing set of makefile rules for tests.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] tests: Move tools under tests/tools/

Posted by Michal Privoznik 11 weeks ago
On 5/7/19 11:13 AM, Daniel P. Berrangé wrote:
> On Mon, May 06, 2019 at 10:01:37AM +0200, Michal Privoznik wrote:
>> There are some scripts/binaries that are not tests themselves but
>> rather fulfill support purpose. Separate them from the rest of
>> the tests.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>> ---
>>   .gitignore                                  |  2 +-
>>   Makefile.am                                 |  2 +-
>>   cfg.mk                                      |  4 +-
>>   configure.ac                                |  1 +
>>   tests/Makefile.am                           | 22 +-----
>>   tests/qemucapabilitiestest.c                |  4 +-
>>   tests/testutils.c                           |  2 +-
>>   tests/{ => tools}/.valgrind.supp            |  0
>>   tests/tools/Makefile.am                     | 85 +++++++++++++++++++++
>>   tests/{ => tools}/check-file-access.pl      |  0
>>   tests/{ => tools}/file_access_whitelist.txt |  0
>>   tests/{ => tools}/group-qemu-caps.pl        |  0
>>   tests/{ => tools}/oomtrace.pl               |  0
>>   tests/{ => tools}/qemucapsprobe.c           |  0
>>   tests/{ => tools}/qemucapsprobemock.c       |  0
>>   tests/{ => tools}/test-wrap-argv.pl         |  2 +-
>>   16 files changed, 98 insertions(+), 26 deletions(-)
>>   rename tests/{ => tools}/.valgrind.supp (100%)
>>   create mode 100644 tests/tools/Makefile.am
>>   rename tests/{ => tools}/check-file-access.pl (100%)
>>   rename tests/{ => tools}/file_access_whitelist.txt (100%)
>>   rename tests/{ => tools}/group-qemu-caps.pl (100%)
>>   rename tests/{ => tools}/oomtrace.pl (100%)
>>   rename tests/{ => tools}/qemucapsprobe.c (100%)
>>   rename tests/{ => tools}/qemucapsprobemock.c (100%)
>>   rename tests/{ => tools}/test-wrap-argv.pl (98%)
> 
> If we're going to re-arrange our tests, then I would really like to
> see the goal be to remove the top level "tests/" directory entirely.
> 
> Instead move the tests under the src/ sub-directory that corresponds
> to the code being tested. eg we should have src/util/virhashtest.c
> alongside src/util/virhash.c  So instead of one giant test dir we
> have everything distributed.
> 
> That way if you are running "make" in the src/ directory, you don't
> need to change dir to do tests. It will also let us split up the
> ever growing set of makefile rules for tests.

How is that? Won't we have to have the rules to build tests in 
src/Makefile.am?

Personally, I find having code along side with the tests even more 
disaranged than what we have now. For some unit tests we can have strict 
naming scheme: virhash.c -> virhashtest.c; but for some more advanced 
tests qemuxml2*test this won't work.

BTW: I've found a way to have Makefile.inc.am and have it working so 
I'll post a v2 shortly.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] tests: Move tools under tests/tools/

Posted by Daniel P. Berrangé 11 weeks ago
On Tue, May 07, 2019 at 12:49:18PM +0200, Michal Privoznik wrote:
> On 5/7/19 11:13 AM, Daniel P. Berrangé wrote:
> > On Mon, May 06, 2019 at 10:01:37AM +0200, Michal Privoznik wrote:
> > > There are some scripts/binaries that are not tests themselves but
> > > rather fulfill support purpose. Separate them from the rest of
> > > the tests.
> > > 
> > > Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> > > ---
> > >   .gitignore                                  |  2 +-
> > >   Makefile.am                                 |  2 +-
> > >   cfg.mk                                      |  4 +-
> > >   configure.ac                                |  1 +
> > >   tests/Makefile.am                           | 22 +-----
> > >   tests/qemucapabilitiestest.c                |  4 +-
> > >   tests/testutils.c                           |  2 +-
> > >   tests/{ => tools}/.valgrind.supp            |  0
> > >   tests/tools/Makefile.am                     | 85 +++++++++++++++++++++
> > >   tests/{ => tools}/check-file-access.pl      |  0
> > >   tests/{ => tools}/file_access_whitelist.txt |  0
> > >   tests/{ => tools}/group-qemu-caps.pl        |  0
> > >   tests/{ => tools}/oomtrace.pl               |  0
> > >   tests/{ => tools}/qemucapsprobe.c           |  0
> > >   tests/{ => tools}/qemucapsprobemock.c       |  0
> > >   tests/{ => tools}/test-wrap-argv.pl         |  2 +-
> > >   16 files changed, 98 insertions(+), 26 deletions(-)
> > >   rename tests/{ => tools}/.valgrind.supp (100%)
> > >   create mode 100644 tests/tools/Makefile.am
> > >   rename tests/{ => tools}/check-file-access.pl (100%)
> > >   rename tests/{ => tools}/file_access_whitelist.txt (100%)
> > >   rename tests/{ => tools}/group-qemu-caps.pl (100%)
> > >   rename tests/{ => tools}/oomtrace.pl (100%)
> > >   rename tests/{ => tools}/qemucapsprobe.c (100%)
> > >   rename tests/{ => tools}/qemucapsprobemock.c (100%)
> > >   rename tests/{ => tools}/test-wrap-argv.pl (98%)
> > 
> > If we're going to re-arrange our tests, then I would really like to
> > see the goal be to remove the top level "tests/" directory entirely.
> > 
> > Instead move the tests under the src/ sub-directory that corresponds
> > to the code being tested. eg we should have src/util/virhashtest.c
> > alongside src/util/virhash.c  So instead of one giant test dir we
> > have everything distributed.
> > 
> > That way if you are running "make" in the src/ directory, you don't
> > need to change dir to do tests. It will also let us split up the
> > ever growing set of makefile rules for tests.
> 
> How is that? Won't we have to have the rules to build tests in
> src/Makefile.am?

Most of the rules will be in the many  src/*/Makefile.in.am files

> Personally, I find having code along side with the tests even more
> disaranged than what we have now. For some unit tests we can have strict
> naming scheme: virhash.c -> virhashtest.c; but for some more advanced tests
> qemuxml2*test this won't work.

We don't need to rename everything - that could be src/qemu/qemuxml2argvtest.c
easily enough

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] tests: Move tools under tests/tools/

Posted by Peter Krempa 11 weeks ago
On Tue, May 07, 2019 at 12:15:54 +0100, Daniel Berrange wrote:
> On Tue, May 07, 2019 at 12:49:18PM +0200, Michal Privoznik wrote:
> > On 5/7/19 11:13 AM, Daniel P. Berrangé wrote:
> > > On Mon, May 06, 2019 at 10:01:37AM +0200, Michal Privoznik wrote:
> > > > There are some scripts/binaries that are not tests themselves but
> > > > rather fulfill support purpose. Separate them from the rest of
> > > > the tests.
> > > > 
> > > > Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
> > > > ---

[...]

> > Personally, I find having code along side with the tests even more
> > disaranged than what we have now. For some unit tests we can have strict
> > naming scheme: virhash.c -> virhashtest.c; but for some more advanced tests
> > qemuxml2*test this won't work.
> 
> We don't need to rename everything - that could be src/qemu/qemuxml2argvtest.c
> easily enough

Ewwww that is super gross. I strongly prefer to keep the tests stashed
into a directory (thus qemu/tests/qemuxml2argvtest.c) to avoid having a
gazillion files in the main directory for the driver.

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] tests: Move tools under tests/tools/

Posted by Michal Privoznik 11 weeks ago
On 5/7/19 1:15 PM, Daniel P. Berrangé wrote:
> On Tue, May 07, 2019 at 12:49:18PM +0200, Michal Privoznik wrote:
>> On 5/7/19 11:13 AM, Daniel P. Berrangé wrote:
>>> On Mon, May 06, 2019 at 10:01:37AM +0200, Michal Privoznik wrote:
>>>> There are some scripts/binaries that are not tests themselves but
>>>> rather fulfill support purpose. Separate them from the rest of
>>>> the tests.
>>>>
>>>> Signed-off-by: Michal Privoznik <mprivozn@redhat.com>
>>>> ---
>>>>    .gitignore                                  |  2 +-
>>>>    Makefile.am                                 |  2 +-
>>>>    cfg.mk                                      |  4 +-
>>>>    configure.ac                                |  1 +
>>>>    tests/Makefile.am                           | 22 +-----
>>>>    tests/qemucapabilitiestest.c                |  4 +-
>>>>    tests/testutils.c                           |  2 +-
>>>>    tests/{ => tools}/.valgrind.supp            |  0
>>>>    tests/tools/Makefile.am                     | 85 +++++++++++++++++++++
>>>>    tests/{ => tools}/check-file-access.pl      |  0
>>>>    tests/{ => tools}/file_access_whitelist.txt |  0
>>>>    tests/{ => tools}/group-qemu-caps.pl        |  0
>>>>    tests/{ => tools}/oomtrace.pl               |  0
>>>>    tests/{ => tools}/qemucapsprobe.c           |  0
>>>>    tests/{ => tools}/qemucapsprobemock.c       |  0
>>>>    tests/{ => tools}/test-wrap-argv.pl         |  2 +-
>>>>    16 files changed, 98 insertions(+), 26 deletions(-)
>>>>    rename tests/{ => tools}/.valgrind.supp (100%)
>>>>    create mode 100644 tests/tools/Makefile.am
>>>>    rename tests/{ => tools}/check-file-access.pl (100%)
>>>>    rename tests/{ => tools}/file_access_whitelist.txt (100%)
>>>>    rename tests/{ => tools}/group-qemu-caps.pl (100%)
>>>>    rename tests/{ => tools}/oomtrace.pl (100%)
>>>>    rename tests/{ => tools}/qemucapsprobe.c (100%)
>>>>    rename tests/{ => tools}/qemucapsprobemock.c (100%)
>>>>    rename tests/{ => tools}/test-wrap-argv.pl (98%)
>>>
>>> If we're going to re-arrange our tests, then I would really like to
>>> see the goal be to remove the top level "tests/" directory entirely.
>>>
>>> Instead move the tests under the src/ sub-directory that corresponds
>>> to the code being tested. eg we should have src/util/virhashtest.c
>>> alongside src/util/virhash.c  So instead of one giant test dir we
>>> have everything distributed.
>>>
>>> That way if you are running "make" in the src/ directory, you don't
>>> need to change dir to do tests. 

I don't know what your workflow is, but I have a terminal open at 
toplevel libvirt.git and then run vim over individual files, e.g.

libvirt.git $ vim src/qemu/qemu_driver.c
libvirt.git $ vim tools/virsh.c

This way I can have tags in libvirt.git and can access them from 
whatever file I'm in. Running binaries is a bit more verbose:

libvirt.git $ ./src/libvirtd --listen

For tests I have a separate terminal open which entered tests:

libvirt.git/tests $

So the benefin of having virhashtest next to virhash.c highly depends on 
one's workflow.

>>> It will also let us split up the
>>> ever growing set of makefile rules for tests.
>>
>> How is that? Won't we have to have the rules to build tests in
>> src/Makefile.am?
> 
> Most of the rules will be in the many  src/*/Makefile.in.am files

Yeah, while the overall number of rules won't change, they'll be spread 
accross multiple Makefiles.

> 
>> Personally, I find having code along side with the tests even more
>> disaranged than what we have now. For some unit tests we can have strict
>> naming scheme: virhash.c -> virhashtest.c; but for some more advanced tests
>> qemuxml2*test this won't work.
> 
> We don't need to rename everything - that could be src/qemu/qemuxml2argvtest.c
> easily enough
> 

Okay, let me send a v2 and see what others think.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] tests: Move tools under tests/tools/

Posted by Andrea Bolognani 11 weeks ago
On Mon, 2019-05-06 at 10:01 +0200, Michal Privoznik wrote:
[...]
> +++ b/configure.ac
> @@ -925,6 +925,7 @@ AC_CONFIG_FILES([\
>          include/libvirt/libvirt-common.h \
>          examples/Makefile \
>          tests/Makefile \
> +        tests/tools/Makefile \

Are we sure we want to have a tests/tools/Makefile.am rather than a
tests/tools/Makefile.inc.am that we include from tests/Makefile.am
here? We seem to be going in the former direction, though I'm not
actually sure what the trade-offs are. CC'ing Dan who introduced
the Makefile.inc.am files in the first place.

[...]
> +++ b/tests/Makefile.am
>  EXTRA_DIST += \
> -	check-file-access.pl \
> -	file_access_whitelist.txt
> +	tools/check-file-access.pl \
> +	tools/file_access_whitelist.txt

These should go into the new Makefile(.inc).am, just like you've
done with .valgrind.supp and oomtrace.pl.


Everything else looks sane from a very cursory looks, but I'd like
to put it through its paces ('make distcheck', 'make rpm', ...)
before ACKing it.

-- 
Andrea Bolognani / Red Hat / Virtualization

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] tests: Move tools under tests/tools/

Posted by Michal Privoznik 11 weeks ago
On 5/6/19 1:58 PM, Andrea Bolognani wrote:
> On Mon, 2019-05-06 at 10:01 +0200, Michal Privoznik wrote:
> [...]
>> +++ b/configure.ac
>> @@ -925,6 +925,7 @@ AC_CONFIG_FILES([\
>>           include/libvirt/libvirt-common.h \
>>           examples/Makefile \
>>           tests/Makefile \
>> +        tests/tools/Makefile \
> 
> Are we sure we want to have a tests/tools/Makefile.am rather than a
> tests/tools/Makefile.inc.am that we include from tests/Makefile.am
> here? We seem to be going in the former direction, though I'm not
> actually sure what the trade-offs are. CC'ing Dan who introduced
> the Makefile.inc.am files in the first place.

I don't think that will work. The problem is that Makefile.inc.am would 
still be included from tests/Makefile.am and thus all the relative paths 
would be evaluated in relation to that. That is the reason why for 
instance src/qemu/Makefile.inc.am lists source files as qemu/qemu_*.c 
and all the libraries/binaries it builds are placed right under src/.

What I'd like to have is for all these tools to live under separate 
directory - even the compiled binaries.
And this is the point where my automake knowledge was weak enough so the 
only solution I was able to come up with was a separate Makefile.

But I'm open to learn something new.

> 
> [...]
>> +++ b/tests/Makefile.am
>>   EXTRA_DIST += \
>> -	check-file-access.pl \
>> -	file_access_whitelist.txt
>> +	tools/check-file-access.pl \
>> +	tools/file_access_whitelist.txt
> 
> These should go into the new Makefile(.inc).am, just like you've
> done with .valgrind.supp and oomtrace.pl.

Shoot. I wanted to move these. Consider fixed.

Michal

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Re: [libvirt] [PATCH] tests: Move tools under tests/tools/

Posted by Ján Tomko 11 weeks ago
On Mon, May 06, 2019 at 01:58:49PM +0200, Andrea Bolognani wrote:
>On Mon, 2019-05-06 at 10:01 +0200, Michal Privoznik wrote:
>[...]
>> +++ b/configure.ac
>> @@ -925,6 +925,7 @@ AC_CONFIG_FILES([\
>>          include/libvirt/libvirt-common.h \
>>          examples/Makefile \
>>          tests/Makefile \
>> +        tests/tools/Makefile \
>
>Are we sure we want to have a tests/tools/Makefile.am rather than a
>tests/tools/Makefile.inc.am that we include from tests/Makefile.am

including a makefile one level up helps paralellism, especialy if
there's not much done in it.

Jano

>here? We seem to be going in the former direction, though I'm not
>actually sure what the trade-offs are. CC'ing Dan who introduced
>the Makefile.inc.am files in the first place.
>
>[...]
>> +++ b/tests/Makefile.am
>>  EXTRA_DIST += \
>> -	check-file-access.pl \
>> -	file_access_whitelist.txt
>> +	tools/check-file-access.pl \
>> +	tools/file_access_whitelist.txt
>
>These should go into the new Makefile(.inc).am, just like you've
>done with .valgrind.supp and oomtrace.pl.
>
>
>Everything else looks sane from a very cursory looks, but I'd like
>to put it through its paces ('make distcheck', 'make rpm', ...)
>before ACKing it.
>
>-- 
>Andrea Bolognani / Red Hat / Virtualization
>
>--
>libvir-list mailing list
>libvir-list@redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list