Code review comment for lp:~nelson-chu/opencompute/add-ocp-cpu-memory-job

Hi Nelson.. Most everything looks good now. There are two remaining issues...

1: the changelog now seems to have duplicate entreies (see the diff below).

I am sorry if I wasn't clear, but you need to put your entries BETWEEN the version header and timestamp lines. For example:

checkbox (1.16.13~OCP) UNRELEASED; urgency=low

  [ Jeff Marcom ]
  * Updated scripts/network script from lp:checkbox

 -- Jeff Marcom <email address hidden> Tue, 5 Nov 2013 11:12:04 -0400

You entry should come after the "checkbox (1.16.13~OCP) line and above the first entry by Jeff Marcom like so:

checkbox (1.16.13~OCP) UNRELEASED; urgency=low

  [ Nelson Chu ]
  * data/whitelists/opencompute-certify-local.whitelist - Added new jobs to
    certification whitelist
  * jobs/TC-001-0002-Platform_Controller_Hub.txt - Added new jobs for PCH
    test cases
  * jobs/local.txt.in - Added job to parse new PCH job file
  * scripts/check_sata_port - new script to verify SATA port speed
  * scripts/check_usb_port - new script to verify USB version

  [ Jeff Marcom ]
  * Updated scripts/network script from lp:checkbox

 -- Jeff Marcom <email address hidden> Tue, 5 Nov 2013 11:12:04 -0400

2: the memory_info script doesn't seem to work... I keep getting this when I try to run it:
bladernr@mini-ubuntu:~/development/nelson-cpu-memory$ sudo ./scripts/memory_info
Fail: Cannot find memory slot

If I run lshw manually, I see memory class objects in the listing:
<node id="bank:0" claimed="true" class="memory" handle="DMI:001F">
       <description>DIMM</description>
       <product>None</product>
       <vendor>None</vendor>
       <physid>0</physid>
       <serial>None</serial>
       <slot>A0</slot>
       <size units="bytes">1073741824</size>
       <width units="bits">64</width>
      </node>

which does list all the required attributes (vendor, description, slot and size).

Can you show me an example of XML from lshw that is correct and works, and an example of passing script output

3: one other thing I just noticed... you do this a lot:

  print("some error message")
  return SOME_NON_ZERO_INTEGER

If you do it this way, your error message will not appear in the checkbox results. Checkbox will add stdout if a test passes and stderr if a test returns a non-zero exit code.

To correct this, ANY message that should be printed when a script exits in an error condition like this:

  print("FAIL: Some necessary criteria was not met")
  return 50

should be instead be printed to stderr like this:
  print("FAIL: Some necessary criteria was not met", file=sys.stderr)
  return 50

This will save your sanity in the long run if a test fails and you don't see any output in the results file.