Re: [OE-core] [PATCH 19/20] oeqa.selftest.wic: Split configuration from code

2016-08-09 Thread Jose Lamego 
Hi Ed, thanks for your quick response.

On 08/09/2016 02:26 AM, Ed Bartosh wrote:
> Hi Jose,
> 
> On Mon, Aug 08, 2016 at 09:23:07AM -0700, Jose Lamego wrote:
>> Improve oeqa-selftest capabilities and UX by placing
>> test configuration features and variables into a separate
>> configuration file.
>>
> 
> Frankly I have mixed feelings about these changes. From the first look
> splitting configuration and code is a good idea. However, looking at the
> result I must say I feel like the test became less readable and harder
> to understand and maintain.
> 
> May be the changes make sense for other tests, but for this one they make
> my life as a maintainer of this one less easy I'd say.
I totally understand you point. But before we drop the idea of this
change for wic, would there be a way to keep the code  understandable
while leveraging on the configuration file to be able to easily try
different input/output combinations without touching the test code
(which is the basic purpose of the change)? Maybe renaming the conf
variables (see below proposals)?
If you still believe there would be no benefits from this change, it
will be better to skip it for wic.
> 
>> [Yocto 9389]
>>
>> Signed-off-by: Jose Lamego 
>> ---
>>  meta/lib/oeqa/selftest/conf/wic.conf |  10 +++
>>  meta/lib/oeqa/selftest/wic.py| 150 
>> ---
>>  2 files changed, 98 insertions(+), 62 deletions(-)
>>  create mode 100644 meta/lib/oeqa/selftest/conf/wic.conf
>>
>> diff --git a/meta/lib/oeqa/selftest/conf/wic.conf 
>> b/meta/lib/oeqa/selftest/conf/wic.conf
>> new file mode 100644
>> index 000..e2afb8d
>> --- /dev/null
>> +++ b/meta/lib/oeqa/selftest/conf/wic.conf
>> @@ -0,0 +1,10 @@
>> +[Wic]
>> +setUpClass_image1 = core-image-minimal
>> +setUpClass_image2 = minimal
> setUpClass_image2 is not used in the code.
> 
>> +setUpClass_image3 = qemux86-directdisk
>> +setUpClass_image4 = directdisk
>> +setUpClass_image5 = wic-image-minimal
> setUpClass_image1 is an image produced by bitbake, but setUpClass_image3
> is a name of wic image. They're not the same. Bitbake image is an input
> for wic and wic image is an output. setUpClass_image5 is again the name
> of the bitbake target, but it differs from _image3. Naming doesn't reflect 
> this fact.
setUpClass_image1 could be renamed to setUpClass_bitbakeImage1
setUpClass_image3 could be renamed to setClass_wicImage
setUpClass_image5 to setUpClass_bitbakeImage2
> 
> 
>> +setUpClass_features = IMAGE_FSTYPES += " hddimg"
>> +  MACHINE_FEATURES_append = " efi"
>> +setUpClass_recipes = syslinux syslinux-native parted-native gptfdisk-native 
>> dosfstools-native mtools-native bmap-tools-native
> This list is used only once in the test. Moving it to config file makes
> it longer to understand what's in the list.
> 
>> diff --git a/meta/lib/oeqa/selftest/wic.py b/meta/lib/oeqa/selftest/wic.py
>> index e550785..dc81457 100644
>> --- a/meta/lib/oeqa/selftest/wic.py
>> +++ b/meta/lib/oeqa/selftest/wic.py
>> @@ -31,6 +31,7 @@ from shutil import rmtree
>>  from oeqa.selftest.base import oeSelfTest
>>  from oeqa.utils.commands import runCmd, bitbake, get_bb_var, runqemu
>>  from oeqa.utils.decorators import testcase
>> +from oeqa.utils.readconfig import conffile
>>  
>>  
>>  class Wic(oeSelfTest):
>> @@ -39,6 +40,18 @@ class Wic(oeSelfTest):
>>  resultdir = "/var/tmp/wic/build/"
>>  image_is_ready = False
>>  
>> +@classmethod
>> +def setUpClass(cls):
>> +# Get test configurations from configuration file
>> +cls.config = conffile(__file__)
>> +cls.image1 = cls.config.get('Wic', 'setUpClass_image1')
>> +cls.image2 = cls.config.get('Wic', 'setUpClass_image2')
>> +cls.image3 = cls.config.get('Wic', 'setUpClass_image3')
>> +cls.image4 = cls.config.get('Wic', 'setUpClass_image4')
>> +cls.image5 = cls.config.get('Wic', 'setUpClass_image5')
>> +cls.features = cls.config.get('Wic', 'setUpClass_features')
>> +cls.recipes = cls.config.get('Wic', 'setUpClass_recipes')
> This looks like a duplication of code and configuration and also hides real
> image names and features making test much less readable.
> 
>> +
>>  def setUpLocal(self):
>>  """This code is executed before each test method."""
>>  self.write_config('IMAGE_FSTYPES += " hddimg"\n'
>> @@ -48,9 +61,8 @@ class Wic(oeSelfTest):
>>  # clean up which can result in the native tools built earlier in
>>  # setUpClass being unavailable.
>>  if not Wic.image_is_ready:
>> -bitbake('syslinux syslinux-native parted-native gptfdisk-native 
>> '
>> -'dosfstools-native mtools-native bmap-tools-native')
>> -bitbake('core-image-minimal')
>> +bitbake(self.recipes)
>> +bitbake(self.image1)
> I'd really prefer to see list of dependencies and the image name right
> in the test code without having to

Re: [OE-core] [PATCH 19/20] oeqa.selftest.wic: Split configuration from code

2016-08-09 Thread Ed Bartosh 
Hi Jose,

On Mon, Aug 08, 2016 at 09:23:07AM -0700, Jose Lamego wrote:
> Improve oeqa-selftest capabilities and UX by placing
> test configuration features and variables into a separate
> configuration file.
> 

Frankly I have mixed feelings about these changes. From the first look
splitting configuration and code is a good idea. However, looking at the
result I must say I feel like the test became less readable and harder
to understand and maintain.

May be the changes make sense for other tests, but for this one they make
my life as a maintainer of this one less easy I'd say.

> [Yocto 9389]
> 
> Signed-off-by: Jose Lamego 
> ---
>  meta/lib/oeqa/selftest/conf/wic.conf |  10 +++
>  meta/lib/oeqa/selftest/wic.py| 150 
> ---
>  2 files changed, 98 insertions(+), 62 deletions(-)
>  create mode 100644 meta/lib/oeqa/selftest/conf/wic.conf
> 
> diff --git a/meta/lib/oeqa/selftest/conf/wic.conf 
> b/meta/lib/oeqa/selftest/conf/wic.conf
> new file mode 100644
> index 000..e2afb8d
> --- /dev/null
> +++ b/meta/lib/oeqa/selftest/conf/wic.conf
> @@ -0,0 +1,10 @@
> +[Wic]
> +setUpClass_image1 = core-image-minimal
> +setUpClass_image2 = minimal
setUpClass_image2 is not used in the code.

> +setUpClass_image3 = qemux86-directdisk
> +setUpClass_image4 = directdisk
> +setUpClass_image5 = wic-image-minimal
setUpClass_image1 is an image produced by bitbake, but setUpClass_image3
is a name of wic image. They're not the same. Bitbake image is an input
for wic and wic image is an output. setUpClass_image5 is again the name
of the bitbake target, but it differs from _image3. Naming doesn't reflect this 
fact.


> +setUpClass_features = IMAGE_FSTYPES += " hddimg"
> +  MACHINE_FEATURES_append = " efi"
> +setUpClass_recipes = syslinux syslinux-native parted-native gptfdisk-native 
> dosfstools-native mtools-native bmap-tools-native
This list is used only once in the test. Moving it to config file makes
it longer to understand what's in the list.

> diff --git a/meta/lib/oeqa/selftest/wic.py b/meta/lib/oeqa/selftest/wic.py
> index e550785..dc81457 100644
> --- a/meta/lib/oeqa/selftest/wic.py
> +++ b/meta/lib/oeqa/selftest/wic.py
> @@ -31,6 +31,7 @@ from shutil import rmtree
>  from oeqa.selftest.base import oeSelfTest
>  from oeqa.utils.commands import runCmd, bitbake, get_bb_var, runqemu
>  from oeqa.utils.decorators import testcase
> +from oeqa.utils.readconfig import conffile
>  
>  
>  class Wic(oeSelfTest):
> @@ -39,6 +40,18 @@ class Wic(oeSelfTest):
>  resultdir = "/var/tmp/wic/build/"
>  image_is_ready = False
>  
> +@classmethod
> +def setUpClass(cls):
> +# Get test configurations from configuration file
> +cls.config = conffile(__file__)
> +cls.image1 = cls.config.get('Wic', 'setUpClass_image1')
> +cls.image2 = cls.config.get('Wic', 'setUpClass_image2')
> +cls.image3 = cls.config.get('Wic', 'setUpClass_image3')
> +cls.image4 = cls.config.get('Wic', 'setUpClass_image4')
> +cls.image5 = cls.config.get('Wic', 'setUpClass_image5')
> +cls.features = cls.config.get('Wic', 'setUpClass_features')
> +cls.recipes = cls.config.get('Wic', 'setUpClass_recipes')
This looks like a duplication of code and configuration and also hides real
image names and features making test much less readable.

> +
>  def setUpLocal(self):
>  """This code is executed before each test method."""
>  self.write_config('IMAGE_FSTYPES += " hddimg"\n'
> @@ -48,9 +61,8 @@ class Wic(oeSelfTest):
>  # clean up which can result in the native tools built earlier in
>  # setUpClass being unavailable.
>  if not Wic.image_is_ready:
> -bitbake('syslinux syslinux-native parted-native gptfdisk-native '
> -'dosfstools-native mtools-native bmap-tools-native')
> -bitbake('core-image-minimal')
> +bitbake(self.recipes)
> +bitbake(self.image1)
I'd really prefer to see list of dependencies and the image name right
in the test code without having to look into configuration file.

The same is true for pretty much all the changes.

>  Wic.image_is_ready = True
>  
>  rmtree(self.resultdir, ignore_errors=True)
> @@ -73,30 +85,35 @@ class Wic(oeSelfTest):
>  @testcase(1211)
>  def test_build_image_name(self):
>  """Test wic create directdisk --image-name core-image-minimal"""
> -self.assertEqual(0, runCmd("wic create directdisk "
> -   "--image-name core-image-minimal").status)
> -self.assertEqual(1, len(glob(self.resultdir + 
> "directdisk-*.direct")))
> +self.assertEqual(
> +0, runCmd("wic create %s --image-name %s"
> +  % (self.image4, self.image1)).status)
> +self.assertEqual(
> +1, len(glob(self.resultdir + "%s-*.direct" % self.image4)))
>  
>

[OE-core] [PATCH 19/20] oeqa.selftest.wic: Split configuration from code

2016-08-08 Thread Jose Lamego 
Improve oeqa-selftest capabilities and UX by placing
test configuration features and variables into a separate
configuration file.

[Yocto 9389]

Signed-off-by: Jose Lamego 
---
 meta/lib/oeqa/selftest/conf/wic.conf |  10 +++
 meta/lib/oeqa/selftest/wic.py| 150 ---
 2 files changed, 98 insertions(+), 62 deletions(-)
 create mode 100644 meta/lib/oeqa/selftest/conf/wic.conf

diff --git a/meta/lib/oeqa/selftest/conf/wic.conf 
b/meta/lib/oeqa/selftest/conf/wic.conf
new file mode 100644
index 000..e2afb8d
--- /dev/null
+++ b/meta/lib/oeqa/selftest/conf/wic.conf
@@ -0,0 +1,10 @@
+[Wic]
+setUpClass_image1 = core-image-minimal
+setUpClass_image2 = minimal
+setUpClass_image3 = qemux86-directdisk
+setUpClass_image4 = directdisk
+setUpClass_image5 = wic-image-minimal
+setUpClass_features = IMAGE_FSTYPES += " hddimg"
+  MACHINE_FEATURES_append = " efi"
+setUpClass_recipes = syslinux syslinux-native parted-native gptfdisk-native 
dosfstools-native mtools-native bmap-tools-native
+
diff --git a/meta/lib/oeqa/selftest/wic.py b/meta/lib/oeqa/selftest/wic.py
index e550785..dc81457 100644
--- a/meta/lib/oeqa/selftest/wic.py
+++ b/meta/lib/oeqa/selftest/wic.py
@@ -31,6 +31,7 @@ from shutil import rmtree
 from oeqa.selftest.base import oeSelfTest
 from oeqa.utils.commands import runCmd, bitbake, get_bb_var, runqemu
 from oeqa.utils.decorators import testcase
+from oeqa.utils.readconfig import conffile
 
 
 class Wic(oeSelfTest):
@@ -39,6 +40,18 @@ class Wic(oeSelfTest):
 resultdir = "/var/tmp/wic/build/"
 image_is_ready = False
 
+@classmethod
+def setUpClass(cls):
+# Get test configurations from configuration file
+cls.config = conffile(__file__)
+cls.image1 = cls.config.get('Wic', 'setUpClass_image1')
+cls.image2 = cls.config.get('Wic', 'setUpClass_image2')
+cls.image3 = cls.config.get('Wic', 'setUpClass_image3')
+cls.image4 = cls.config.get('Wic', 'setUpClass_image4')
+cls.image5 = cls.config.get('Wic', 'setUpClass_image5')
+cls.features = cls.config.get('Wic', 'setUpClass_features')
+cls.recipes = cls.config.get('Wic', 'setUpClass_recipes')
+
 def setUpLocal(self):
 """This code is executed before each test method."""
 self.write_config('IMAGE_FSTYPES += " hddimg"\n'
@@ -48,9 +61,8 @@ class Wic(oeSelfTest):
 # clean up which can result in the native tools built earlier in
 # setUpClass being unavailable.
 if not Wic.image_is_ready:
-bitbake('syslinux syslinux-native parted-native gptfdisk-native '
-'dosfstools-native mtools-native bmap-tools-native')
-bitbake('core-image-minimal')
+bitbake(self.recipes)
+bitbake(self.image1)
 Wic.image_is_ready = True
 
 rmtree(self.resultdir, ignore_errors=True)
@@ -73,30 +85,35 @@ class Wic(oeSelfTest):
 @testcase(1211)
 def test_build_image_name(self):
 """Test wic create directdisk --image-name core-image-minimal"""
-self.assertEqual(0, runCmd("wic create directdisk "
-   "--image-name core-image-minimal").status)
-self.assertEqual(1, len(glob(self.resultdir + "directdisk-*.direct")))
+self.assertEqual(
+0, runCmd("wic create %s --image-name %s"
+  % (self.image4, self.image1)).status)
+self.assertEqual(
+1, len(glob(self.resultdir + "%s-*.direct" % self.image4)))
 
 @testcase(1212)
 def test_build_artifacts(self):
 """Test wic create directdisk providing all artifacts."""
-bbvars = dict((var.lower(), get_bb_var(var, 'core-image-minimal')) \
-for var in ('STAGING_DATADIR', 'DEPLOY_DIR_IMAGE',
-'STAGING_DIR_NATIVE', 'IMAGE_ROOTFS'))
+bbvars = dict((var.lower(), get_bb_var(var, self.image1))
+  for var in ('STAGING_DATADIR', 'DEPLOY_DIR_IMAGE',
+  'STAGING_DIR_NATIVE', 'IMAGE_ROOTFS'))
 status = runCmd("wic create directdisk "
 "-b %(staging_datadir)s "
 "-k %(deploy_dir_image)s "
 "-n %(staging_dir_native)s "
 "-r %(image_rootfs)s" % bbvars).status
 self.assertEqual(0, status)
-self.assertEqual(1, len(glob(self.resultdir + "directdisk-*.direct")))
+self.assertEqual(1, len(glob(self.resultdir + "%s-*.direct"
+ % self.image4)))
 
 @testcase(1157)
 def test_gpt_image(self):
-"""Test creation of core-image-minimal with gpt table and UUID boot"""
-self.assertEqual(0, runCmd("wic create directdisk-gpt "
-   "--image-name core-image-minimal").status)
-self.assertEqual(1, len(glob(self.resultdir +