On Mon, 2021-11-29 at 15:50 +0100, Martin Kletzander wrote: > On Mon, Nov 29, 2021 at 10:41:51PM +0800, Luke Yue wrote: > > On Fri, 2021-11-26 at 16:44 +0100, Martin Kletzander wrote: > > > On Wed, Nov 10, 2021 at 10:24:31PM +0800, Luke Yue wrote: > > > > Signed-off-by: Luke Yue <lukedyue@xxxxxxxxx> > > > > --- > > > > tests/virshtest.c | 96 > > > > +++++++++++++++++++++++++++++++++++++++++++++++ > > > > 1 file changed, 96 insertions(+) > > > > > > > > > > This makes the tests very fragile. It would be even easier to do > > > the > > > test using the internal APIs, similarly to how we do it with the > > > qemuhotplugtest. > > > > > > > Thanks for the explanation, I will look into how qemuhotplugtest is > > implemented. > > > > > > diff --git a/tests/virshtest.c b/tests/virshtest.c > > > > index af2a70f5fb..8e5b397420 100644 > > > > --- a/tests/virshtest.c > > > > +++ b/tests/virshtest.c > > > > @@ -160,6 +160,8 @@ static char *custom_uri; > > > > "--connect", \ > > > > custom_uri > > > > > > > > +# define TEST_XML_PATH abs_top_builddir > > > > "/../examples/xml/test" > > > > + > > > > static int testCompareListDefault(const void *data > > > > G_GNUC_UNUSED) > > > > { > > > > const char *const argv[] = { VIRSH_DEFAULT, "list", NULL }; > > > > @@ -437,6 +439,88 @@ static int testIOThreadPin(const void > > > > *data > > > > G_GNUC_UNUSED) > > > > return testCompareOutputLit(exp, "", NULL, argv); > > > > } > > > > > > > > +static int testCompareDetachDevice(const void *data > > > > G_GNUC_UNUSED) > > > > +{ > > > > + const char *const argv[] = { VIRSH_CUSTOM, "detach-device > > > > fc5\ > > > > + " TEST_XML_PATH > > > > "/testdevif.xml;\ > > > > + detach-device fc5\ > > > > + " TEST_XML_PATH > > > > "/testdevdiskcdrom.xml;\ > > > > + detach-device fc5\ > > > > + " TEST_XML_PATH > > > > "/testdevsound.xml;\ > > > > + detach-device fc5\ > > > > + " TEST_XML_PATH > > > > "/testdevhostdev.xml;\ > > > > + detach-device fc5\ > > > > + " TEST_XML_PATH > > > > "/testdevlease.xml;\ > > > > + detach-device fc5\ > > > > + " TEST_XML_PATH > > > > "/testdevcontroller.xml;\ > > > > + detach-device fc5\ > > > > + " TEST_XML_PATH > > > > "/testdevfs.xml;\ > > > > + detach-device fc5\ > > > > + " TEST_XML_PATH > > > > "/testdevrng.xml;\ > > > > + detach-device fc5\ > > > > + " TEST_XML_PATH > > > > "/testdevmem.xml;\ > > > > + detach-device fc5\ > > > > + " TEST_XML_PATH > > > > "/testdevshmem.xml;\ > > > > + detach-device fc5\ > > > > + " TEST_XML_PATH > > > > "/testdevwatchdog.xml;\ > > > > + detach-device fc5\ > > > > + " TEST_XML_PATH > > > > "/testdevinput.xml;\ > > > > + detach-device fc5\ > > > > + " TEST_XML_PATH > > > > "/testdevvsock.xml;\ > > > > + detach-device fc5\ > > > > + " TEST_XML_PATH > > > > "/testdevtpm.xml", > > > > + NULL }; > > > > + const char *exp = > > > > +"Device detached successfully\n\n\ > > > > +Device detached successfully\n\n\ > > > > +Device detached successfully\n\n\ > > > > +Device detached successfully\n\n\ > > > > +Device detached successfully\n\n\ > > > > +Device detached successfully\n\n\ > > > > +Device detached successfully\n\n\ > > > > +Device detached successfully\n\n\ > > > > +Device detached successfully\n\n\ > > > > +Device detached successfully\n\n\ > > > > +Device detached successfully\n\n\ > > > > +Device detached successfully\n\n\ > > > > +Device detached successfully\n\n\ > > > > +Device detached successfully\n\n"; > > > > > > If any of these fails it will be a pain to figure out which one > > > it > > > was > > > if the error message does not include the name. Splitting these > > > is > > > definitely the way to go. > > > > > > > + return testCompareOutputLit(exp, "", NULL, argv); > > > > +} > > > > + > > > > +static int testCompareDetachDeviceError(const void *data > > > > G_GNUC_UNUSED) > > > > +{ > > > > + const char *const argv[] = { VIRSH_CUSTOM, "detach-device > > > > fc5\ > > > > + " TEST_XML_PATH > > > > "/testdevtpm.xml;\ > > > > + detach-device fc5\ > > > > + " TEST_XML_PATH > > > > "/testdevtpm.xml;\ > > > > + detach-device fc5 --live\ > > > > + " TEST_XML_PATH > > > > "/testdevmemballoon.xml", > > > > + NULL }; > > > > + const char *exp = > > > > +"Device detached successfully\n\n\n\n"; > > > > + const char *error_msg = > > > > +"error: Failed to detach device from " TEST_XML_PATH > > > > "/testdevtpm.xml\n\ > > > > +error: device not found: matching tpm device not found\n\ > > > > +error: Failed to detach device from " TEST_XML_PATH > > > > "/testdevmemballoon.xml\n\ > > > > +error: Operation not supported: detach of device 'memballoon' > > > > on > > > > running domain is not supported\n"; > > > > > > It'd be also nicer to read and write these tests if they did not > > > rely > > > on > > > the output just like this and instead used the internal APIs. > > > > Should I create a new file for these tests? As this file is for > > `virsh` > > test but no for test using internal APIs? > > > > I would go with "generichotplugtest" or something like that. > Thanks, will reimplement these tests with internal APIs in next version.