diary-15aug2013.md

15 Aug 2013

Ubuntu Code of Conduct

I noticed that in Launchpad, it said I hadn't signed the "Ubuntu Code of Conduct" and asked jpich if I need to worry about that. She said:

Nope, that's nothing to worry about, it only matters if you plan on contributing to Ubuntu itself, or participate in its community.

Comparing patch sets on Gerrit

I discovered today that I can compare diffs of patch sets on Gerrit directly on the website. I don't have to download the patchs to my computer and run diff manually. You can compare patch sets on the Gerrit website by going to "Old Version History" and selecting one of the patch sets you want to compare to (by default "Old Version History" is set to "Base"). Then go down to the other patch and click on "Diff All Side-by-Side" or "Diff All Unified", whatever is your preferred view.

"Diff All Side-by-Side" shows the different versions next to each other, with changes highlighted in color. "Diff All Unified" shows changes in a single file with added lines prefixed with "+" and removed lines prefixed with "-".

So if you wanted to compare Patch Set 3 and Patch Set 4 of a patch on Gerrit, you could go to "Old Version History" and select "Patch Set 3". Then scroll down to Patch Set 4, click on the arrow net to Patch Set 4 and click on "Diff All Side-by-Side" or "Diff All Unified".

I heard you can also run the command

$ git review -m <review#>,<rev1>-<rev2>

People fixing bugs I reported

Previously, I reported the bug "MongoDB PyMongo username/password warnings occur when running tests"

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

sileht wrote a patch to fix this issue:

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

And it was merged today.

Discussion about unit test design

I asked on #openstack-101 what test cases do you want to cover when you write unit tests.

jpich said:

Expected working paths and failure paths sound like a reasonable start to me. Maybe if you expect to have boundaries for something somewhere, it's usually good to have tests around those too to check they actually work (off by one errors). Unusual arguments (e.g. empty list, group by for a list of one, etc) may be good as well

litong said:

not sure about the context, but I think unittest should test every line of the code that you write.

which should cover the boundary parameters already if different values makes your code goes different path.

so if your test cases cover all the path, then you are good.

I think that is why the unittest code coverage stats is useful.

Following up on litong's comment, I asked him how to check coverage stats in Ceilometer:

I have not see it myself, swift unittest script by default always shows that stats. probably try --cover-tests

I mean I have not seen it in Ceilometer unit test. but run-tests.sh does have some parameters for coverage.

swift .unittest script always show that stats, I think you can try to use --cover-tests option and see what it does for you.

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

jd's Patch Set 4

jd submitted Patch Set 4, which has some code in it that makes it clear that the SQL driver doesn't support groupby on the source field.

I've tried source, since you wrote a test, and it's very hard to do

there's a sourceassoc table which is wrong and badly written that needs to be joined and all

it's just not as trivial as the user/resource/project fields

He also added a test for checking invalid fields in the groupby parameter; the code was added to tests/storage/base.py. He also made a few stylistic changes to the whitespace in the storage tests that I wrote.

Doug Hellmann had some comments in his code review:

In the drivers that don't support groupby, please raise a NotImplemented exception if a group value is provided. That way callers will get an error, instead of invalid data.

Mike Perez made a comment about avoiding duplicated code in his code review.

Discussion with jd

Since I'm working together with jd on the SQL Alchemy patch, I was worried about messing up the git history. I asked jd if I should use the command

git review -d patch####

to download his latest patch before I start working on it.

He said:

Yes, doing that will build a local branch that you can then checkout, hack on, git commit --amend and git review to repush. This is exactly what I did yesterday.

I was also wondering how much code goes in each patch. I thought there was already a lot of code in the current SQL Alchemy patch.

jd said:

Definitely! The patch as it is is enough. All can be added to it is more tests, but I wouldn't add more code for more feature, it's sufficient.

I had a question on when to use lists and tuples. For example, in the storage tests I wrote, in the prepare_data() method, the variable test_sample_list is actually a tuple.

jd said:

Tuple are faster, but are immutable. So use tuple if you can, otherwise use list.

Next steps for blueprint

• Modify the prototype of all get_meter_statistics() functions to accept the groupby parameter, raising NotImplementedError in the first place.

  jd may have done most of this already.

• Add tests to check group by in combination with the period parameter.

  • group by single field with period
  • group by single metadata field with period
  • group by single field with query filter and period
  • group by single metadata field with query filter and period (this is the example in the wiki)

• Add tests to check for incorrect values like groupby=['user_id', 'bogus']

  jd wrote a test for this in Patch Set 4.

• Revise unit tests so that results from get_meter_statistics() don't have to be in a specific order.

• Start implementing MongoDB driver

• Write patch to rename sources field to source in the SQL driver, so that the SQL driver tests pass

  jd decided that this is too hard and is tabling support for groupby with the source field for now.