diary-13aug2013.md

13 Aug 2013

Stuff to learn

• Todo: Learn more about unit testing
• Todo: Learn more about mocking and how it works in Ceilometer
• Todo: Learn more about coverage testing and how it works in Ceilometer

Other people patching the bugs I reported

• jd fixed the bug I reported "tests for Nova notifier fail because of nonexistent method"

  Bug report: https://bugs.launchpad.net/ceilometer/+bug/1211532

  Patch: https://review.openstack.org/#/c/41628/

  jd had an interesting remark about why this bug was categorized as "critical":

  It stops as soon as this test set fails, because tests are run with 2 differents commands this way:

  1. run nova_tests/

  2. run tests/

  (see run-tests.sh)

  Since command 1 fails, it stops.

  If there are failures in the nova tests, then the other Ceilometer tests are never run, and no one can test their patches.

• sileht submitted a patch to address the bug I reported earlier.

  Bug report: https://bugs.launchpad.net/ceilometer/+bug/1210674

  Patch: https://review.openstack.org/#/c/41719/

  I reviewed the patch by cherry picking it and running the Ceilometer test suite. I found that all the MongoDB warnings were eliminated except for 3.

  Ceilometer test output: https://gist.github.com/terriyu/6237785

New bug found in alarm threshold evaluation tests

When I run the Ceilometer test suite, I get failures in tests/alarm/test_threshold_evaluation.py

The test failures all give the same traceback:

 Traceback (most recent call last):
 File "/opt/stack/ceilometer/tests/alarm/test_threshold_evaluation.py", line 88, in test_retry_transient_api_failure
 self.evaluator.evaluate()
 File "/opt/stack/ceilometer/ceilometer/alarm/threshold_evaluation.py", line 205, in evaluate
 LOG.info(_('initiating evaluation cycle on %d alarms') %
 NameError: global name '_' is not defined

The full test output is here: https://gist.github.com/terriyu/6230038

thomasm and sandywalsh confirmed the error.

I submitted a bug report: https://bugs.launchpad.net/ceilometer/+bug/1211975

Group by blueprint

• group by blueprint on Launchpad: https://blueprints.launchpad.net/ceilometer/+spec/api-group-by
• group by blueprint discussion on the OpenStack wiki: https://wiki.openstack.org/wiki/Ceilometer/blueprints/api-group-by

Discussion with jd

• When I submitted Patch Set 1 to Gerrit, Jenkins gave me a weird message in the py27 tests results:

  2013-08-13 04:51:26.082 | Warning: you are leaving 124 commits behind, not connected to
  2013-08-13 04:51:26.082 | any of your branches:
  2013-08-13 04:51:26.083 |
  2013-08-13 04:51:26.083 |   ab0ea27 Change the default sort direction of mongodb to ascending
  2013-08-13 04:51:26.083 |   916e166 Change test_post_alarm case in test_alarm_scenarios
  2013-08-13 04:51:26.084 |   0975ae3 Merge "Add support for API message localization"
  2013-08-13 04:51:26.084 |   046eefb Merge "s/alarm/alarm_id/ in alarm notification"
  2013-08-13 04:51:26.084 |  ... and 120 more.
  

jd says:

> Don't worry about that, your patch's ok. It's just a side effect warning from
> how Jenkins plays with git.

• I noticed that in the statistics tests that each attribute was tested in a separate assert statement. So I copied that. It makes the unit tests looks longer, but I assume this is the correct thing to do? It's tempting to run assert on a list like

  self.assertEqual([r.group for r in results], ["group-1", "group-2", "group-3"]
  

  I asked jd to comment on this:

  First, that could work but could be dangerous since that would rely on the group being in a particular order. Best way is to write the same thing with sets:

  self.assertEqual(set(r.group for r in results), set(["group-1", "group-2", "group-3"]))
  

• I noticed that sometimes there are lists in the source code where you leave a comma after the last item in the list. For example, in https://github.com/openstack/ceilometer/blob/master/tests/storage/base.py#L669

  resource_metadata={'display_name': 'test-volume',
                     'tag': 'self.counter',
                     },
  

  I found that kind of weird, but I copied it in my test class. I asked jd why people use this style.

  Yes, the reason is that if you wan to add one more element, you just have to insert a line, and don't have to touch the other ones. While not mandatory, it has the nice effect to not pollute the output of 'git blame' for example.

New patchset

Patchset 2 here: https://review.openstack.org/#/c/41597/

py27 Ceilometer test output: https://gist.github.com/terriyu/6230064

• I figured out why my tests kept passing even though they weren't supposed to. I implemented the tests in tests/storage/base.py, but I didn't put the class definition for my new class in tests/storage/impl_mongodb.py, tests/storage/impl_sqlalchemy.py, and tests/storage/hbase.py.

  Actually, I should probably not put the class definition in tests/storage/hbase.py because we don't plan to support group by in HBase right now.

• Following jd's suggestion, I tried to come up with a good naming scheme for the new groupby parameters and attributes.

  My idea is that the parameter passed to get_meter_statistics() is called groupby. In the results returned by get_meter_statistics(), each entry of the list is the data for one of the groups. Each entry of the list has an attribute called group and it contains a list identifying the group. If that's confusing, I could call it group_id

  An example using the groupby parameter:

  results = list(self.conn.get_meter_statistics(f, groupby=['user_id', 'resource-id']))
  

  An example using the group attribute

  r1 = results[0]
  self.assertEqual(r1.group, ['user-1', 'resource-1'])
  

• I added some more tests to check group by with query filters.