Code review comment for lp://qastaging/~david4dev/dmedia/udisks

Revision history for this message
Jason Gerard DeRose (jderose) wrote :

So I've been thinking on this a bit: how about for now we add calls to self.skipTest() for these udisk related tests that are causing trouble, and go ahead and merge this to trunk?

There is some critical schema stuff in here, and I'd like to have it in trunk so we can play with it day-to-day, knock off any rough edges over the next month.

As far as the tests, long term we probably need an adapter that sits between dmedia code and dbus. Normally we'll have, say, a LiveAdapter that makes real dbus calls, but in the tests we'll have a DummyAdapter that returns canned responses setup by the test.

I think one of the most critical bits is probably how we create the drive ID. I'm liking your _drive_id() function, but a few things we should investigate before declaring the v1 schema final:

 * Would it be safe to make the drive ID from the DriveSerial alone?

 * Regardless what values go into the drive ID, we need to make sure these are all stable values, ie, is their some interpretation by udisk or the kernel that may change between versions, or are these exact values from the physical device?

 * Will we have access to these values on other platforms, and will the values be uniform, produce the same hash? I'd say most important is making sure removable drives are giving a uniform ID, so if you took a removable drive from an Ubuntu system running dmeida to an OSX system running dmedia, everything works as expected.

What do you think? I'll go ahead and merge this to trunk, and I can add the skipTest() for the the tests that don't work on my system (easier for me to do as my system is the problem one).

Again, great work, David! I'm quite proud of you for digging into the schema so aggressively!

review: Approve

« Back to merge proposal