| Message ID | 20180525005839.11556-4-crosa@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 05/24 20:58, Cleber Rosa wrote: > This patch adds a few simple behavior tests for VNC. These tests > introduce manipulation of the QEMUMachine arguments, by setting > the arguments, instead of adding to the existing ones. I'm confused by this. The code uses 'add_args', so it does add to the arguments, no? > > Signed-off-by: Cleber Rosa <crosa@redhat.com> > --- > tests/acceptance/test_vnc.py | 50 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 50 insertions(+) > create mode 100644 tests/acceptance/test_vnc.py > > diff --git a/tests/acceptance/test_vnc.py b/tests/acceptance/test_vnc.py > new file mode 100644 > index 0000000000..9d9a35cf55 > --- /dev/null > +++ b/tests/acceptance/test_vnc.py > @@ -0,0 +1,50 @@ Copyright header is missing here too. Fam > +from avocado_qemu import Test > + > + > +class Vnc(Test): Should VncTest be a better class name? > + """ > + :avocado: enable > + :avocado: tags=vnc,quick > + """ > + def test_no_vnc(self): > + self.vm.add_args('-nodefaults', '-S') > + self.vm.launch() > + self.assertFalse(self.vm.qmp('query-vnc')['return']['enabled']) > + > + def test_no_vnc_change_password(self): > + self.vm.add_args('-nodefaults', '-S') > + self.vm.launch() > + self.assertFalse(self.vm.qmp('query-vnc')['return']['enabled']) > + set_password_response = self.vm.qmp('change', > + device='vnc', > + target='password', > + arg='new_password') > + self.assertIn('error', set_password_response) > + self.assertEqual(set_password_response['error']['class'], > + 'GenericError') > + self.assertEqual(set_password_response['error']['desc'], > + 'Could not set password') > + > + def test_vnc_change_password_requires_a_password(self): > + self.vm.add_args('-nodefaults', '-S', '-vnc', ':0') > + self.vm.launch() > + self.assertTrue(self.vm.qmp('query-vnc')['return']['enabled']) > + set_password_response = self.vm.qmp('change', > + device='vnc', > + target='password', > + arg='new_password') > + self.assertIn('error', set_password_response) > + self.assertEqual(set_password_response['error']['class'], > + 'GenericError') > + self.assertEqual(set_password_response['error']['desc'], > + 'Could not set password') > + > + def test_vnc_change_password(self): > + self.vm.add_args('-nodefaults', '-S', '-vnc', ':0,password') > + self.vm.launch() > + self.assertTrue(self.vm.qmp('query-vnc')['return']['enabled']) > + set_password_response = self.vm.qmp('change', > + device='vnc', > + target='password', > + arg='new_password') > + self.assertEqual(set_password_response['return'], {}) > -- > 2.17.0 > Fam
On 05/25/2018 01:37 AM, Fam Zheng wrote: > On Thu, 05/24 20:58, Cleber Rosa wrote: >> This patch adds a few simple behavior tests for VNC. These tests >> introduce manipulation of the QEMUMachine arguments, by setting >> the arguments, instead of adding to the existing ones. > > I'm confused by this. The code uses 'add_args', so it does add to the arguments, > no? > And you should be. I changed the code to just add args, and forgot to update the commit message. If a better example comes up that requires setting arguments, I'll get back to this. >> >> Signed-off-by: Cleber Rosa <crosa@redhat.com> >> --- >> tests/acceptance/test_vnc.py | 50 ++++++++++++++++++++++++++++++++++++ >> 1 file changed, 50 insertions(+) >> create mode 100644 tests/acceptance/test_vnc.py >> >> diff --git a/tests/acceptance/test_vnc.py b/tests/acceptance/test_vnc.py >> new file mode 100644 >> index 0000000000..9d9a35cf55 >> --- /dev/null >> +++ b/tests/acceptance/test_vnc.py >> @@ -0,0 +1,50 @@ > > Copyright header is missing here too. > Indeed. > Fam > >> +from avocado_qemu import Test >> + >> + >> +class Vnc(Test): > > Should VncTest be a better class name? > I'm favoring simpler names. If you think about the complete test names, it's already too verbose IMO: "test_vnc.Vnc.test_no_vnc". That's actually an interesting point: how would you feel about dropping the "test_" prefix from the file names? - Cleber. >> + """ >> + :avocado: enable >> + :avocado: tags=vnc,quick >> + """ >> + def test_no_vnc(self): >> + self.vm.add_args('-nodefaults', '-S') >> + self.vm.launch() >> + self.assertFalse(self.vm.qmp('query-vnc')['return']['enabled']) >> + >> + def test_no_vnc_change_password(self): >> + self.vm.add_args('-nodefaults', '-S') >> + self.vm.launch() >> + self.assertFalse(self.vm.qmp('query-vnc')['return']['enabled']) >> + set_password_response = self.vm.qmp('change', >> + device='vnc', >> + target='password', >> + arg='new_password') >> + self.assertIn('error', set_password_response) >> + self.assertEqual(set_password_response['error']['class'], >> + 'GenericError') >> + self.assertEqual(set_password_response['error']['desc'], >> + 'Could not set password') >> + >> + def test_vnc_change_password_requires_a_password(self): >> + self.vm.add_args('-nodefaults', '-S', '-vnc', ':0') >> + self.vm.launch() >> + self.assertTrue(self.vm.qmp('query-vnc')['return']['enabled']) >> + set_password_response = self.vm.qmp('change', >> + device='vnc', >> + target='password', >> + arg='new_password') >> + self.assertIn('error', set_password_response) >> + self.assertEqual(set_password_response['error']['class'], >> + 'GenericError') >> + self.assertEqual(set_password_response['error']['desc'], >> + 'Could not set password') >> + >> + def test_vnc_change_password(self): >> + self.vm.add_args('-nodefaults', '-S', '-vnc', ':0,password') >> + self.vm.launch() >> + self.assertTrue(self.vm.qmp('query-vnc')['return']['enabled']) >> + set_password_response = self.vm.qmp('change', >> + device='vnc', >> + target='password', >> + arg='new_password') >> + self.assertEqual(set_password_response['return'], {}) >> -- >> 2.17.0 >> > > Fam >
On Fri, May 25, 2018 at 12:27:46PM -0400, Cleber Rosa wrote: > > > On 05/25/2018 01:37 AM, Fam Zheng wrote: > > On Thu, 05/24 20:58, Cleber Rosa wrote: > >> This patch adds a few simple behavior tests for VNC. These tests > >> introduce manipulation of the QEMUMachine arguments, by setting > >> the arguments, instead of adding to the existing ones. > > > > I'm confused by this. The code uses 'add_args', so it does add to the arguments, > > no? > > > > And you should be. I changed the code to just add args, and forgot to > update the commit message. If a better example comes up that requires > setting arguments, I'll get back to this. > > >> > >> Signed-off-by: Cleber Rosa <crosa@redhat.com> > >> --- > >> tests/acceptance/test_vnc.py | 50 ++++++++++++++++++++++++++++++++++++ > >> 1 file changed, 50 insertions(+) > >> create mode 100644 tests/acceptance/test_vnc.py > >> > >> diff --git a/tests/acceptance/test_vnc.py b/tests/acceptance/test_vnc.py > >> new file mode 100644 > >> index 0000000000..9d9a35cf55 > >> --- /dev/null > >> +++ b/tests/acceptance/test_vnc.py > >> @@ -0,0 +1,50 @@ > > > > Copyright header is missing here too. > > > > Indeed. > > > Fam > > > >> +from avocado_qemu import Test > >> + > >> + > >> +class Vnc(Test): > > > > Should VncTest be a better class name? > > > > I'm favoring simpler names. If you think about the complete test names, > it's already too verbose IMO: "test_vnc.Vnc.test_no_vnc". > > That's actually an interesting point: how would you feel about dropping > the "test_" prefix from the file names? I would like this. The file is already inside a tests/acceptance directory. About the class name, I would be less surprised when reading the code if it was called "VncTest", but I won't object to "Vnc" if you prefer it.
On 05/29/2018 10:32 AM, Eduardo Habkost wrote: > On Fri, May 25, 2018 at 12:27:46PM -0400, Cleber Rosa wrote: >> >> >> On 05/25/2018 01:37 AM, Fam Zheng wrote: >>> On Thu, 05/24 20:58, Cleber Rosa wrote: >>>> This patch adds a few simple behavior tests for VNC. These tests >>>> introduce manipulation of the QEMUMachine arguments, by setting >>>> the arguments, instead of adding to the existing ones. >>> >>> I'm confused by this. The code uses 'add_args', so it does add to the arguments, >>> no? >>> >> >> And you should be. I changed the code to just add args, and forgot to >> update the commit message. If a better example comes up that requires >> setting arguments, I'll get back to this. >> >>>> >>>> Signed-off-by: Cleber Rosa <crosa@redhat.com> >>>> --- >>>> tests/acceptance/test_vnc.py | 50 ++++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 50 insertions(+) >>>> create mode 100644 tests/acceptance/test_vnc.py >>>> >>>> diff --git a/tests/acceptance/test_vnc.py b/tests/acceptance/test_vnc.py >>>> new file mode 100644 >>>> index 0000000000..9d9a35cf55 >>>> --- /dev/null >>>> +++ b/tests/acceptance/test_vnc.py >>>> @@ -0,0 +1,50 @@ >>> >>> Copyright header is missing here too. >>> >> >> Indeed. >> >>> Fam >>> >>>> +from avocado_qemu import Test >>>> + >>>> + >>>> +class Vnc(Test): >>> >>> Should VncTest be a better class name? >>> >> >> I'm favoring simpler names. If you think about the complete test names, >> it's already too verbose IMO: "test_vnc.Vnc.test_no_vnc". >> >> That's actually an interesting point: how would you feel about dropping >> the "test_" prefix from the file names? > > I would like this. The file is already inside a tests/acceptance > directory. > > About the class name, I would be less surprised when reading the > code if it was called "VncTest", but I won't object to "Vnc" if > you prefer it. > We already gained one simplification by dropping the "test_" file prefix, so I wouldn't mind adding the "Test" suffix to the test classes. Now, before I do, consider that when reading the code you'll find: class Vnc(Test): ... Which IMO makes it pretty clear that this is a test class. And as you said it yourself, those files are already in a tests/acceptance directory. So back to you (for the last time, I promise): would you like me to add the Test suffix to all test classes? - Cleber.
On Tue, May 29, 2018 at 10:59:37AM -0400, Cleber Rosa wrote: > > > On 05/29/2018 10:32 AM, Eduardo Habkost wrote: > > On Fri, May 25, 2018 at 12:27:46PM -0400, Cleber Rosa wrote: > >> > >> > >> On 05/25/2018 01:37 AM, Fam Zheng wrote: > >>> On Thu, 05/24 20:58, Cleber Rosa wrote: > >>>> This patch adds a few simple behavior tests for VNC. These tests > >>>> introduce manipulation of the QEMUMachine arguments, by setting > >>>> the arguments, instead of adding to the existing ones. > >>> > >>> I'm confused by this. The code uses 'add_args', so it does add to the arguments, > >>> no? > >>> > >> > >> And you should be. I changed the code to just add args, and forgot to > >> update the commit message. If a better example comes up that requires > >> setting arguments, I'll get back to this. > >> > >>>> > >>>> Signed-off-by: Cleber Rosa <crosa@redhat.com> > >>>> --- > >>>> tests/acceptance/test_vnc.py | 50 ++++++++++++++++++++++++++++++++++++ > >>>> 1 file changed, 50 insertions(+) > >>>> create mode 100644 tests/acceptance/test_vnc.py > >>>> > >>>> diff --git a/tests/acceptance/test_vnc.py b/tests/acceptance/test_vnc.py > >>>> new file mode 100644 > >>>> index 0000000000..9d9a35cf55 > >>>> --- /dev/null > >>>> +++ b/tests/acceptance/test_vnc.py > >>>> @@ -0,0 +1,50 @@ > >>> > >>> Copyright header is missing here too. > >>> > >> > >> Indeed. > >> > >>> Fam > >>> > >>>> +from avocado_qemu import Test > >>>> + > >>>> + > >>>> +class Vnc(Test): > >>> > >>> Should VncTest be a better class name? > >>> > >> > >> I'm favoring simpler names. If you think about the complete test names, > >> it's already too verbose IMO: "test_vnc.Vnc.test_no_vnc". > >> > >> That's actually an interesting point: how would you feel about dropping > >> the "test_" prefix from the file names? > > > > I would like this. The file is already inside a tests/acceptance > > directory. > > > > About the class name, I would be less surprised when reading the > > code if it was called "VncTest", but I won't object to "Vnc" if > > you prefer it. > > > > We already gained one simplification by dropping the "test_" file > prefix, so I wouldn't mind adding the "Test" suffix to the test classes. > > Now, before I do, consider that when reading the code you'll find: > > class Vnc(Test): > ... > > Which IMO makes it pretty clear that this is a test class. And as you > said it yourself, those files are already in a tests/acceptance directory. Yes, he context makes it clear it's a test class, but the pattern is still unusual because class names are normally meaningful when they are referenced elsewhere. However, in this case it doesn't matter too much because there are no references to "Vnc" in the rest of the code. > > So back to you (for the last time, I promise): would you like me to add > the Test suffix to all test classes? It's up to you. I'd add them, but I won't complain too loudly if you prefer to not add it. :)
On 05/29/2018 01:52 PM, Eduardo Habkost wrote: > On Tue, May 29, 2018 at 10:59:37AM -0400, Cleber Rosa wrote: >> >> >> On 05/29/2018 10:32 AM, Eduardo Habkost wrote: >>> On Fri, May 25, 2018 at 12:27:46PM -0400, Cleber Rosa wrote: >>>> >>>> >>>> On 05/25/2018 01:37 AM, Fam Zheng wrote: >>>>> On Thu, 05/24 20:58, Cleber Rosa wrote: >>>>>> This patch adds a few simple behavior tests for VNC. These tests >>>>>> introduce manipulation of the QEMUMachine arguments, by setting >>>>>> the arguments, instead of adding to the existing ones. >>>>> >>>>> I'm confused by this. The code uses 'add_args', so it does add to the arguments, >>>>> no? >>>>> >>>> >>>> And you should be. I changed the code to just add args, and forgot to >>>> update the commit message. If a better example comes up that requires >>>> setting arguments, I'll get back to this. >>>> >>>>>> >>>>>> Signed-off-by: Cleber Rosa <crosa@redhat.com> >>>>>> --- >>>>>> tests/acceptance/test_vnc.py | 50 ++++++++++++++++++++++++++++++++++++ >>>>>> 1 file changed, 50 insertions(+) >>>>>> create mode 100644 tests/acceptance/test_vnc.py >>>>>> >>>>>> diff --git a/tests/acceptance/test_vnc.py b/tests/acceptance/test_vnc.py >>>>>> new file mode 100644 >>>>>> index 0000000000..9d9a35cf55 >>>>>> --- /dev/null >>>>>> +++ b/tests/acceptance/test_vnc.py >>>>>> @@ -0,0 +1,50 @@ >>>>> >>>>> Copyright header is missing here too. >>>>> >>>> >>>> Indeed. >>>> >>>>> Fam >>>>> >>>>>> +from avocado_qemu import Test >>>>>> + >>>>>> + >>>>>> +class Vnc(Test): >>>>> >>>>> Should VncTest be a better class name? >>>>> >>>> >>>> I'm favoring simpler names. If you think about the complete test names, >>>> it's already too verbose IMO: "test_vnc.Vnc.test_no_vnc". >>>> >>>> That's actually an interesting point: how would you feel about dropping >>>> the "test_" prefix from the file names? >>> >>> I would like this. The file is already inside a tests/acceptance >>> directory. >>> >>> About the class name, I would be less surprised when reading the >>> code if it was called "VncTest", but I won't object to "Vnc" if >>> you prefer it. >>> >> >> We already gained one simplification by dropping the "test_" file >> prefix, so I wouldn't mind adding the "Test" suffix to the test classes. >> >> Now, before I do, consider that when reading the code you'll find: >> >> class Vnc(Test): >> ... >> >> Which IMO makes it pretty clear that this is a test class. And as you >> said it yourself, those files are already in a tests/acceptance directory. > > Yes, he context makes it clear it's a test class, but the pattern > is still unusual because class names are normally meaningful when > they are referenced elsewhere. However, in this case it doesn't > matter too much because there are no references to "Vnc" in the > rest of the code. > > >> >> So back to you (for the last time, I promise): would you like me to add >> the Test suffix to all test classes? > > It's up to you. I'd add them, but I won't complain too loudly if > you prefer to not add it. :) > OK. I'll listen to my intuition here, as it's telling me pretty loudly that it will be seen as a good thing "soon", when we're looking at the result of hundreds of tests! ;) - Cleber.
diff --git a/tests/acceptance/test_vnc.py b/tests/acceptance/test_vnc.py new file mode 100644 index 0000000000..9d9a35cf55 --- /dev/null +++ b/tests/acceptance/test_vnc.py @@ -0,0 +1,50 @@ +from avocado_qemu import Test + + +class Vnc(Test): + """ + :avocado: enable + :avocado: tags=vnc,quick + """ + def test_no_vnc(self): + self.vm.add_args('-nodefaults', '-S') + self.vm.launch() + self.assertFalse(self.vm.qmp('query-vnc')['return']['enabled']) + + def test_no_vnc_change_password(self): + self.vm.add_args('-nodefaults', '-S') + self.vm.launch() + self.assertFalse(self.vm.qmp('query-vnc')['return']['enabled']) + set_password_response = self.vm.qmp('change', + device='vnc', + target='password', + arg='new_password') + self.assertIn('error', set_password_response) + self.assertEqual(set_password_response['error']['class'], + 'GenericError') + self.assertEqual(set_password_response['error']['desc'], + 'Could not set password') + + def test_vnc_change_password_requires_a_password(self): + self.vm.add_args('-nodefaults', '-S', '-vnc', ':0') + self.vm.launch() + self.assertTrue(self.vm.qmp('query-vnc')['return']['enabled']) + set_password_response = self.vm.qmp('change', + device='vnc', + target='password', + arg='new_password') + self.assertIn('error', set_password_response) + self.assertEqual(set_password_response['error']['class'], + 'GenericError') + self.assertEqual(set_password_response['error']['desc'], + 'Could not set password') + + def test_vnc_change_password(self): + self.vm.add_args('-nodefaults', '-S', '-vnc', ':0,password') + self.vm.launch() + self.assertTrue(self.vm.qmp('query-vnc')['return']['enabled']) + set_password_response = self.vm.qmp('change', + device='vnc', + target='password', + arg='new_password') + self.assertEqual(set_password_response['return'], {})
This patch adds a few simple behavior tests for VNC. These tests introduce manipulation of the QEMUMachine arguments, by setting the arguments, instead of adding to the existing ones. Signed-off-by: Cleber Rosa <crosa@redhat.com> --- tests/acceptance/test_vnc.py | 50 ++++++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) create mode 100644 tests/acceptance/test_vnc.py