Merge lp:~bladernr/checkbox/945178-make-auto-cd-read-test into lp:checkbox

[Jeff Lane ]

Created an automated version of optical/read to remove the last manual job from the server whitelists.

[lp:~bladernr/checkbox/945178-make-auto-cd-read-test]

1319. By Jeff Lane  on 2012-03-15

Reverted feature to keep tests ordered, as the sortkey attribute causes
undesirable secondary effects.

1320. By Sylvain Pineau on 2012-03-16

Show the UF invalidation warning if all test cases are unchecked from the right click menu (LP: #956757)

[TienFu Chen (ctf)]

I know this patch is to the job file, not the script, optical_read_test.
But I take a look at optical_read_test, found few issue should be taken care in the script.

command "optical_read_test /dev/cdrom" work fine while /dev/cdrom is available.

Few exceptions here:
1)
command: optical_read_test /dev/cdrom (but /dev/cdrom is not existed actually).
command passed and return 0.

2)
command: optical_read_test
command passed and return 0.

3)
Not sure the device name /dev/cdrom is always reliable. I would suggest we use the device name from "wodim --devices"
Sample output is like:
wodim: Overview of accessible drives (1 found) :
-------------------------------------------------------------------------
 0 dev='/dev/sg1' rwrw-- : 'Slimtype' 'DVD A DS8A4S'
-------------------------------------------------------------------------

We can take the dev='xxxx' part.

4) optical_read_test can handle multi cd-rom test.(yes,I don't understand why so many cd-roms installed on server too), But the job files should be changed too. We may have job name like: optical/read_*

So now, I would say 1) 2) should be fixed. 3) 4) can be taken care later.

review: Needs Fixing

[lp:~bladernr/checkbox/945178-make-auto-cd-read-test]

1321. By Jeff Lane  on 2012-03-16

Merged latest trunk

1322. By Jeff Lane  on 2012-03-16

* Created automated version of optical/read for server testing
  Fixed issues with optical_read_test script:
  - test could pass if /dev/cdrom did not exist
  - test could pass if /dev/cdrom was inaccessible
  - test could pass if no optical device was passed in (LP: #945178)

[Jeff Lane  (bladernr)]

> I know this patch is to the job file, not the script, optical_read_test.
> But I take a look at optical_read_test, found few issue should be taken care
> in the script.

No worries, those are issues that need to be addressed. See below:

> Few exceptions here:
> 1)
> command: optical_read_test /dev/cdrom (but /dev/cdrom is not existed
> actually).
> command passed and return 0.

Yeah, that was a flaw in the original script design. Now if DEVICE passed in doesn't exist (/lib/udev/cdrom_id will error if DEVICE does not exist or is inaccessible), the test will spit out an error message and exit with a FAIL.

> 2)
> command: optical_read_test
> command passed and return 0.

Heh, yeah, this was broke too. Changed it to use argparse and if you don't specify at least one DEVICE, the script will throw a parser.error and exit with a FAIL.

> 3)
> Not sure the device name /dev/cdrom is always reliable. I would suggest we use
> the device name from "wodim --devices"
> Sample output is like:
> wodim: Overview of accessible drives (1 found) :
> -------------------------------------------------------------------------
> 0 dev='/dev/sg1' rwrw-- : 'Slimtype' 'DVD A DS8A4S'
> -------------------------------------------------------------------------
>
> We can take the dev='xxxx' part.

At a minimum, /dev/cdrom should always be present on a system with an optical drive and that should be a symlink to /dev/sr[0-9]. If a device has an optical drive and is missing at least /dev/cdrom, there's a problem. For example:

$ ls -l /dev/cd*
lrwxrwxrwx 1 root root 3 Mar 16 19:20 /dev/cdrom -> sr0
lrwxrwxrwx 1 root root 3 Mar 16 19:20 /dev/cdrw -> sr0

You CAN access the device via the sr device, but I don't think you can access it via the sg:

$ /lib/udev/cdrom_id /dev/sr0
ID_CDROM=1
ID_CDROM_CD=1
ID_CDROM_CD_R=1
ID_CDROM_CD_RW=1
ID_CDROM_DVD=1
ID_CDROM_MRW=1
ID_CDROM_MRW_W=1
$ /lib/udev/cdrom_id /dev/sg0
unable to open '/dev/sg0'

> 4) optical_read_test can handle multi cd-rom test.(yes,I don't understand why
> so many cd-roms installed on server too), But the job files should be changed
> too. We may have job name like: optical/read_*

We used to have this, actually, with the drives. However, it's pretty much a guarantee that we will never certify a machine that has more than 1 optical drive. In fact, outside of special order systems (and even that's questionable) you'll have difficulty even finding systems for sale with more than one optical drive, if any.

> So now, I would say 1) 2) should be fixed. 3) 4) can be taken care later.

1 and 2 are fixed. I don't think 3 is really an issue, as I said, if a system with an optical drive doesn't have a minimum of the common /dev/cdrom symlink there's a kernel or driver issue that needs to be exposed and investigated. As for 4, we did have that, removed it because it's highly unlikely to ever need that, but we can always revisit that later and change it back. The script will certainly handle multiple devices, so as you noted, the only change would be to the jobs and whitelists.

review: Needs Resubmitting

[TienFu Chen (ctf)]

> You CAN access the device via the sr device, but I don't think you can access it via the sg:
>
> $ /lib/udev/cdrom_id /dev/sr0
> ID_CDROM=1
> ID_CDROM_CD=1
> ID_CDROM_CD_R=1
> ID_CDROM_CD_RW=1
> ID_CDROM_DVD=1
> ID_CDROM_MRW=1
> ID_CDROM_MRW_W=1
> $ /lib/udev/cdrom_id /dev/sg0
> unable to open '/dev/sg0'
True, /dev/sgx is character device, whereas /dev/srx is block device. Thanks for the correction.

> 1 and 2 are fixed. I don't think 3 is really an issue, as I said, if a system
> with an optical drive doesn't have a minimum of the common /dev/cdrom symlink
> there's a kernel or driver issue that needs to be exposed and investigated.
> As for 4, we did have that, removed it because it's highly unlikely to ever
> need that, but we can always revisit that later and change it back. The
> script will certainly handle multiple devices, so as you noted, the only
> change would be to the jobs and whitelists.
Cool, thanks for the update.

review: Approve

=== modified file 'debian/changelog'
--- debian/changelog	2012-03-16 22:49:13 +0000
+++ debian/changelog	2012-03-16 23:20:25 +0000
@@ -15,6 +15,11 @@
     confusing and verbose. (LP: #949745) 
   * Removed dependency on bluetooth/detect-output on the 
     suspend/suspend_advanced job. (LP: #955375)
+  * Created automated version of optical/read for server testing
+    Fixed issues with optical_read_test script:
+    - test could pass if /dev/cdrom did not exist
+    - test could pass if /dev/cdrom was inaccessible
+    - test could pass if no optical device was passed in (LP: #945178)
 
   [Marc Tardif]
   * Linted qt_interface which had a few syntax errors (LP: #949957)
@@ -30,7 +35,7 @@
   * Show the UF invalidation warning if all test cases are unchecked from the
     right click menu (LP: #956757)
 
- -- Jeff Lane <[e-mail]>  Wed, 14 Mar 2012 18:10:57 -0400
+ -- Jeff Lane <[e-mail]>  Fri, 16 Mar 2012 19:14:09 -0400
 
 checkbox (0.13.4) precise; urgency=low
 

=== modified file 'jobs/optical.txt.in'
--- jobs/optical.txt.in	2012-02-15 16:20:14 +0000
+++ jobs/optical.txt.in	2012-03-16 23:20:25 +0000
@@ -23,6 +23,15 @@
  VERIFICATION:
      Were you able to view files on the disk and either open a file to read or copy a file to your home directory?
 
+plugin: shell
+name: optical/read-automated
+requires:
+ device.category == 'CDROM'
+ user: root
+command: optical_read_test /dev/cdrom
+description:
+ This is an automated version of optical/read. It assumes you have already inserted a data CD into your optical drive prior to running Checkbox.
+
 plugin: manual
 name: optical/cdrom-write
 requires:

=== modified file 'scripts/optical_read_test'
--- scripts/optical_read_test	2011-06-28 10:11:51 +0000
+++ scripts/optical_read_test	2012-03-16 23:20:25 +0000
@@ -6,6 +6,7 @@
 import filecmp
 import shutil
 
+from argparse import ArgumentParser
 from subprocess import Popen, PIPE
 
 DEFAULT_DIR = '/tmp/checkbox.optical'
@@ -82,15 +83,24 @@
 def get_capabilities(device):
     cmd = "%s %s" % (CDROM_ID, device)
     capabilities = _command_out(cmd)
-    return capabilities.split('\n')
+    return capabilities
 
-def main(args):
+def main():
     tests = []
     return_values = []
 
-    for device in args:
+    parser = ArgumentParser()
+    parser.add_argument("device", nargs='+',
+                        help='Specify an optical device or list of devices such as /dev/cdrom')
+    args = parser.parse_args()
+
+    for device in args.device:
+        
         capabilities = get_capabilities(device)
-        for capability in capabilities:
+        if not capabilities:
+            print "Unable to get capabilities of %s" % device
+            return 1
+        for capability in capabilities.split('\n'):
             if capability[:3] == 'ID_':
                 cap = capability[3:-2]
                 if cap == 'CDROM' or cap == 'CDROM_DVD':
@@ -105,5 +115,5 @@
     return False in return_values
 
 if __name__ == "__main__":
-    sys.exit(main(sys.argv[1:]))
+    sys.exit(main())