Merge lp:~pwlars/qa-iso-static-validation-test/static-nosetests into lp:qa-iso-static-validation-test

[Paul Larson]

Consider this more of a proof of concept. It *should* work, but there's
a lot of cleanup I'd like to do. Still, we can go ahead and merge it
and start getting the benefit of xunit output right away unless someone
disagrees. I fixed a few opportunistic things along the way, and got
tired of dealing with all the logging output stuff. Really, *all* of
these need to be properly converted so that they are messages passed
from unit test failures rather than logging messages anyway. I did some
of them, but got hand cramps along the way. :)

[Joe Talbott (joetalbott)]

Looks good to me. I think I'd make the environment variables (esp. NAME) a bit more specific to avoid any ambiguity.

review: Approve

[Gema Gomez (gema)]

Can you guys add nuclearbob to the review, we need to make sure the changes are utah friendly!

[Paul Larson (pwlars)]

> Looks good to me. I think I'd make the environment variables (esp. NAME) a
> bit more specific to avoid any ambiguity.
Yeah, good point. I was just preserving the original names as I hacked through it to prove it would work. I'll change them. There's quite a lot of cleanup to be done here still, but I may save some of that for later as this diff is already getting a bit large. Probably one of the main things to note here is the logic inversion for the skip cases since I couldn't use the decorator, but they should be all correct. I tested it locally and had the same number of skips before and after.

[Para Siva (psivaa)]

I tried the change briefly with one of the tests today and skipping seemed working fine.
http://10.189.74.2:8080/view/Quantal/view/ISO%20Testing/job/quantal-desktop-amd64_static_validation/30/console

Still did not try nosetests --with-xunit though

[lp:~pwlars/qa-iso-static-validation-test/static-nosetests]

19. By Paul Larson on 2012-09-21

Change environment var NAME to ISONAME

[Paul Larson (pwlars)]

changed NAME to ISONAME

[Para Siva (psivaa)]

review: Approve

[Max Brustkern (nuclearbob)]

For the most part, I like these changes. The only thing I might take issue with is the replacement of logging statements with print. If we're going to be using this internally in utah, it's better to use the logging facility to integrate. Right now, we're not using it internally, so print makes more sense since it's just running standalone.

That brings me to something that's not an issue with this, but something I think we should consider. We now have an ISO class in UTAH. I looked at making these tests methods of the class, but I didn't want to remove the ability to run them separately. As I've thought about it, to me, it makes sense to make each test a method of the class, I.E., iso.testa, iso.testb, and then have a nose/unittest compatible class which just instantiates an ISO and each test case runs ISO.testa, iso.testb, etc. I'd like to hear whether that approach makes sense to other people. That way, we can easily do validation as part of provisioning, and we still have a script to run it separately and produce the output. Alternately, the ISO class could contain a unittest.TestCase object, and the script could just call that. That would require no synchronization to be maintained between the class and the test script, but it would make the test script a little bare as well.

Finally, is there a strict need for the iso we're testing to be named correctly? Could we extract its relevant information (series, arch, type) from the iso itself (using the iso class in utah or otherwise) and then generate the URL from that? It's a little more work, but it's also a little more flexible.

[Paul Larson (pwlars)]

> For the most part, I like these changes. The only thing I might take issue
> with is the replacement of logging statements with print. If we're going to
> be using this internally in utah, it's better to use the logging facility to
> integrate. Right now, we're not using it internally, so print makes more
> sense since it's just running standalone.
As mentioned, this was a quick hack, mostly to prove it would work. However, I don't think I remember actually replacing any logging statements with print. I do know of one instance where I replaced a "unittest.fail" (Which I'm pretty sure wouldn't have actually worked anyway) with a print, and this is not ideal but at least it wouldn't choke if we got to this point. What I did do, is replace some of the logging messages (more to do here) with a test result message string so that the person reviewing the logs can have a chance at understanding what went wrong without reading the source of the test.

>
> That brings me to something that's not an issue with this, but something I
> think we should consider. We now have an ISO class in UTAH. I looked at
> making these tests methods of the class, but I didn't want to remove the
> ability to run them separately. As I've thought about it, to me, it makes
> sense to make each test a method of the class, I.E., iso.testa, iso.testb, and
> then have a nose/unittest compatible class which just instantiates an ISO and
> each test case runs ISO.testa, iso.testb, etc. I'd like to hear whether that
> approach makes sense to other people. That way, we can easily do validation
> as part of provisioning, and we still have a script to run it separately and
> produce the output. Alternately, the ISO class could contain a
> unittest.TestCase object, and the script could just call that. That would
> require no synchronization to be maintained between the class and the test
> script, but it would make the test script a little bare as well.
In this case we don't actually want to provision anything. But otherwise, this sounds interesting. I'd like to hear more about it, but maybe as an email rather than on a code review?

>
> Finally, is there a strict need for the iso we're testing to be named
> correctly? Could we extract its relevant information (series, arch, type)
> from the iso itself (using the iso class in utah or otherwise) and then
> generate the URL from that? It's a little more work, but it's also a little
> more flexible.
That's a possibility, you'd want to take a look at .disk/info in the iso image and make sure that it. Here's an example from an old iso I had lying around:
Ubuntu-Server 12.10 "Quantal Quetzal" - Alpha amd64+mac (20120903.2)
So as long as they don't ever change *that* format, we should be able to get everything we need from it :)

Since there don't seem to be any major objections, I'll go ahead and merge this, start fixing up the 12 or so static test jobs in jenkins to pull the xunit results, and we can look at some of these other cleanups as we go.

=== modified file 'iso_static_validation.py'
--- iso_static_validation.py	2012-09-18 16:16:16 +0000
+++ iso_static_validation.py	2012-09-21 16:09:19 +0000
@@ -21,7 +21,6 @@
 #
 
 import os
-import argparse
 import sys
 import logging
 import urllib2
@@ -32,63 +31,55 @@
 import tempfile
 import shutil
 
-# Defaults
-DEFAULT_URL = 'http://cdimage.ubuntu.com/'
-ARCHIVE_TEST_URL = 'http://people.canonical.com/~ubuntu-archive/testing/'
-
-#constants
-ONE_MB_BLOCK = 2 ** 20
-PRECISE_SIZE_LIMIT_MB = 737148928
-QUANTAL_SIZE_LIMIT_MB = 838860800
-
-#Read the iso name and initialise global variables
-parser = argparse.ArgumentParser()
-parser.add_argument('--isoroot')
-parser.add_argument('--name')
-args = parser.parse_args()
-
-logging.basicConfig(level=logging.INFO)
-
-if args.name == None or '.iso' not in args.name or args.isoroot == None:
-    print('The usage:', sys.argv[0],
-        '--name release-variant-arch.iso --isoroot ~/iso_location')
-    sys.exit(1)
-else:
-    st_name = args.name.rstrip('.iso')
-    try:
-        (st_release, st_variant, st_arch) = st_name.rsplit('-')
-    except ValueError:
-        sys.exit("Image name be in the form of release-variant-arch.iso")
-    logging.debug("The file being tested is: '%s'", args.name)
-
-#Set a flag for ubiquity based images
-if st_variant == 'desktop' or st_variant == 'dvd':
-    ubiquity_image = True
-else:
-    ubiquity_image = False
 
 
 class TestValidateISO(unittest.TestCase):
     @classmethod
     def setUpClass(cls):
+        # Defaults
+        cls.DEFAULT_URL = 'http://cdimage.ubuntu.com/'
+        cls.ARCHIVE_TEST_URL = 'http://people.canonical.com/~ubuntu-archive/testing/'
+
+        #constants
+        cls.ONE_MB_BLOCK = 2 ** 20
+        cls.PRECISE_SIZE_LIMIT_MB = 737148928
+        cls.QUANTAL_SIZE_LIMIT_MB = 838860800
+
+        #Read the iso name and initialise global variables
+        cls.isoroot=os.getenv('ISOROOT')
+        cls.name=os.getenv('ISONAME')
+
+
+        if cls.name == None or '.iso' not in cls.name or cls.isoroot == None:
+            print("Please see the README")
+            sys.exit(1)
+        else:
+            cls.st_name = cls.name.rstrip('.iso')
+            try:
+                (cls.st_release, cls.st_variant, cls.st_arch) = cls.st_name.rsplit('-')
+            except ValueError:
+                sys.exit("Image name must be in the form of release-variant-arch.iso")
+
+        #Set a flag for ubiquity based images
+        if cls.st_variant == 'desktop' or cls.st_variant == 'dvd':
+            cls.ubiquity_image = True
+        else:
+            cls.ubiquity_image = False
         try:
             cls.temp_dir = tempfile.mkdtemp()
         except OSError:
-            logging.error("Creating temp dirrectory failed")
-            unittest.fail("Setup error")
-
+            print("Creating temp dirrectory failed")
+            raise
+        
     def setUp(self):
-        self.iso_name = args.name
-        self.block_size = ONE_MB_BLOCK
-        self.iso_location = os.path.join(args.isoroot, args.name)
-        self.st_release = st_release
-        self.st_variant = st_variant
-        self.st_arch = st_arch
+        self.iso_name = self.name
+        self.block_size = self.ONE_MB_BLOCK
+        self.iso_location = os.path.join(self.isoroot, self.name)
 
         if self.st_release != 'precise':
-            self.url = DEFAULT_URL
+            self.url = self.DEFAULT_URL
         else:
-            self.url = os.path.join(DEFAULT_URL, 'precise')
+            self.url = os.path.join(self.DEFAULT_URL, 'precise')
 
         if self.st_variant == 'desktop':
             self.url = os.path.join(self.url, 'daily-live')
@@ -98,14 +89,14 @@
             self.url = os.path.join(self.url, 'daily')
         elif self.st_variant == 'server':
             if self.st_release == 'precise':
-                self.url = os.path.join(DEFAULT_URL, 'ubuntu-server',
+                self.url = os.path.join(self.DEFAULT_URL, 'ubuntu-server',
                                         'precise', 'daily')
             else:  # current dev release
-                self.url = os.path.join(DEFAULT_URL, 'ubuntu-server', 'daily')
+                self.url = os.path.join(self.DEFAULT_URL, 'ubuntu-server', 'daily')
         else:
             self.fail("Image name be in the form of release-variant-arch.iso")
 
-        self.probs_url = os.path.join(ARCHIVE_TEST_URL, self.st_release +
+        self.probs_url = os.path.join(self.ARCHIVE_TEST_URL, self.st_release +
             '_probs.html')
 
     # Test if the sha check sum of the iso matches that is given in the server
@@ -115,9 +106,9 @@
         try:
             fh = urllib2.urlopen(sha256sum_url)
         except urllib2.HTTPError, e:
-            logging.error("Failed to fetch URL '%s': %s. Aborting!",
-                          sha256sum_url, e)
-            self.fail("Failed to fetch sha checksum from the server")
+            self.fail(
+                "Failed to fetch URL '{0}': {1}. Aborting!".format(
+                ha256sum_url, e))
 
         # Process the file (contains a number of entries containing iso
         # names and the corresponding checksums)
@@ -129,11 +120,10 @@
         try:
             iso_sha256 = current_iso_digest[self.iso_name]
         except KeyError:
-            logging.error("No digest found for image '%s'", self.iso_name)
-            self.fail("The iso does not have a matching digest")
+            self.fail("No digest found for image '{0}'".format(
+                elf.iso_name))
 
         # Validate checksum of file fname against sha256 hash
-        logging.debug("Calculating hash for file '%s'", self.iso_name)
         sha256 = hashlib.sha256()
         with open(self.iso_location) as f:
             while True:
@@ -142,9 +132,9 @@
                     break
                 sha256.update(data)
         sha256 = sha256.hexdigest()
-        logging.debug("Expected Checksum: '%s'", iso_sha256)
-        logging.debug("Local File Checksum: '%s'", sha256)
-        self.assertEqual(iso_sha256, sha256)
+        self.assertEqual(iso_sha256, sha256,
+            "\nExpected checksum {0}\nLocal file checksum {1}".format(
+            iso_sha256, sha256))
 
     # Test if the list file in the repository matches the content of the iso
     def test_files_list(self):
@@ -161,26 +151,25 @@
         try:
             list_repository = urllib2.urlopen(list_url)
         except urllib2.HTTPError, e:
-            logging.error("Failed to fetch URL '%s': %s . Aborting!",
-                          list_url, e)
-            self.fail("Failed to fetch files list from the server")
+            self.fail("Failed to fetch URL '{0}': {1} . Aborting!".format(
+                list_url, e))
 
-        logging.debug('Extracting list file info from ISO')
         cmd = ["bsdtar", "-tf", self.iso_location]
         output = subprocess.Popen(cmd, stdout=subprocess.PIPE,
                                   stderr=subprocess.PIPE)
         (stdout, stderr) = output.communicate()
-        logging.debug('Checking for error in extracting the file list')
-        self.assertEqual(stderr, '')
+        self.assertEqual(stderr, '', "Error extracting the file list")
 
         for list_entry in list_repository:
-            logging.debug('Checking that each list entry is in the iso')
-            self.assertIn(list_entry[1:].rstrip(), stdout)
+            self.assertIn(list_entry[1:].rstrip(), stdout,
+                "{0} missing from iso".format(list_entry[1:]))
+        #FIXME: This should probably generate a complete list and save
+        #it as an artifact.
 
     # Test if the manfest is the same as that is given in the server
-    @unittest.skipUnless(ubiquity_image,
-                        "manifest only for ubiquity based images")
     def test_manifest(self):
+        if(self.ubiquity_image==False):
+            self.skipTest("manifest only for ubiquity based images")
         manifest_url = os.path.join(self.url, 'current',
                                     self.st_release + '-' +
                                     self.st_variant + '-' +
@@ -190,16 +179,17 @@
         except urllib2.HTTPError, e:
             logging.error("Failed to fetch URL '%s': %s . Aborting!",
                           manifest_url, e)
-            self.fail("Failed to fetch manifest file from the server")
+            self.fail(
+                "Failed to fetch manifest file from the server:\n{0}".format(e))
 
-        logging.debug('Extracting manifest from the iso')
         cmd = ["bsdtar", "-xf", self.iso_location, "-C", self.temp_dir,
                "casper/filesystem.manifest"]
         output = subprocess.Popen(cmd, stdout=subprocess.PIPE,
                                   stderr=subprocess.PIPE)
         (stdout, stderr) = output.communicate()
         logging.debug('Check for error in extracting manifest info from iso')
-        self.assertEqual(stderr, '')
+        self.assertEqual(stderr, '',
+            "Error extracting manifest from iso:\n{0}".format(stderr))
 
         manifest_path = os.path.join(self.temp_dir,
                                      'casper/filesystem.manifest')
@@ -243,9 +233,9 @@
             self.assertIn(id_server, build_info_line)
 
     # Test if  wubi is present for desktop or dvd and it is a valid PE
-    @unittest.skipUnless(st_variant == 'desktop' and
-                         st_arch != 'powerpc', "Skipping non ubiquity images")
     def test_wubi(self):
+        if(self.st_variant != 'desktop' or self.st_arch == 'powerpc'):
+            self.skipTest("Skipping non ubiquity images")
         logging.debug('Extracting wubi from the desktop images')
         cmd = ["bsdtar", "-xf", self.iso_location, "-C",
                                 self.temp_dir, "./wubi.exe"]
@@ -266,9 +256,9 @@
         self.assertIn(" PE32 executable (GUI)", stdout)
 
     # Test if the relevant files are present in the iso for desktop iso
-    @unittest.skipUnless(st_variant == 'desktop' and
-                         st_arch != 'powerpc', "Skipping for non desktop iso")
     def test_files_ubiquity(self):
+        if(self.st_variant != 'desktop' or self.st_arch == 'powerpc'):
+            self.skipTest("Skipping for non desktop iso")
         cmd = ["bsdtar", "-tf", self.iso_location]
         output = subprocess.Popen(cmd, stdout=subprocess.PIPE,
                                   stderr=subprocess.PIPE)
@@ -281,9 +271,9 @@
             self.assertIn(list_server[2:].rstrip(), stdout)
 
     # Test if the relevant important files are present in the iso for dvd iso
-    @unittest.skipUnless(st_variant == 'dvd',
-                        "This file list test is only specific for dvd")
     def test_files_ubiquity_dvd(self):
+        if(self.st_variant != 'dvd'):
+            self.skipTest("This file list test is only specific for dvd")
         cmd = ["bsdtar", "-tf", self.iso_location]
         output = subprocess.Popen(cmd, stdout=subprocess.PIPE,
                                   stderr=subprocess.PIPE)
@@ -300,8 +290,9 @@
     # file_list_di for i386 and amd64
     # file_list_di_powerpc for powerpc and amd64+mac
     # Test is skipped for ubiquity based images
-    @unittest.skipIf(ubiquity_image, "Skipping for ubiquity images")
     def test_files_di(self):
+        if(self.ubiquity_image):
+            self.skipTest("Skipping for ubiquity images")
         cmd = ["bsdtar", "-tf", self.iso_location]
         output = subprocess.Popen(cmd, stdout=subprocess.PIPE,
                                   stderr=subprocess.PIPE)
@@ -319,9 +310,9 @@
             self.assertIn(list_server[2:].rstrip(), stdout)
 
     # Test if vmlinuz is a valid executable
-    @unittest.skipUnless(ubiquity_image and  st_arch != 'powerpc',
-                         "vmlinuz is present only for ubiquity based images")
     def test_vmlinuz(self):
+        if(self.ubiquity_image == False or self.st_arch == 'powerpc'):
+            self.skipTest("vmlinuz is present only for ubiquity based images")
         cmd = ["bsdtar", "-xf", self.iso_location, "-C",
                                 self.temp_dir, "casper/vmlinuz"]
         output = subprocess.Popen(cmd, stdout=subprocess.PIPE,
@@ -342,8 +333,9 @@
 
     # Test if filesystem.squashfs is present and
     # if that is a squash fs for ubiquity based images
-    @unittest.skipUnless(ubiquity_image, "Skipping for non ubiquity images")
     def test_filesystem_squashfs(self):
+        if(self.ubiquity_image == False):
+            self.skipTest("Skipping for non ubiquity images")
         logging.debug('Extracting the filesystem.squashfs')
         cmd = ["bsdtar", "-xf", self.iso_location, "-C",
                                  self.temp_dir, "casper/filesystem.squashfs"]
@@ -375,8 +367,9 @@
 
     # Test if the report.html exists for the d-i based images
     # and that it does not have any conflicting packages
-    @unittest.skipIf(ubiquity_image, "Skipping for ubiquity based images")
     def test_report_html(self):
+        if(self.ubiquity_image == True):
+            self.skipTest("Skipping for ubiquity based images")
         report_url = os.path.join(self.url, 'current', 'report.html')
         try:
             fh_report_page = urllib2.urlopen(report_url).read()
@@ -404,11 +397,11 @@
 
         if(self.st_release == 'precise'):
             # check if the size of the image fits a 703MB cd for precise
-            self.assertTrue(statinfo.st_size < PRECISE_SIZE_LIMIT_MB,
+            self.assertTrue(statinfo.st_size < self.PRECISE_SIZE_LIMIT_MB,
                 "Size: %s" % (statinfo.st_size))
         else:
             # check if the size of the image fits a 800MB cd for quantal
-            self.assertTrue(statinfo.st_size < QUANTAL_SIZE_LIMIT_MB,
+            self.assertTrue(statinfo.st_size < self.QUANTAL_SIZE_LIMIT_MB,
                 "Size: %s" % (statinfo.st_size))
 
     # clean up
@@ -418,7 +411,6 @@
             shutil.rmtree(cls.temp_dir, ignore_errors=False)
         except OSError:
             logging.error("Removing the temp directory failed")
-            unittest.fail("Cleanup error")
 
 if __name__ == '__main__':
     from test import test_support