Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Ofono template: Add missing properties to SimManager interface #21
Conversation
martinpitt
reviewed
Jun 5, 2016
| @@ -47,6 +49,19 @@ def load(mock, parameters): | ||
| mock.AddModem(parameters.get('ModemName', 'ril_0'), {}) | ||
| +def newSimSerial(): |
martinpitt
reviewed
Jun 5, 2016
| @@ -47,6 +49,19 @@ def load(mock, parameters): | ||
| mock.AddModem(parameters.get('ModemName', 'ril_0'), {}) | ||
| +def newSimSerial(): | ||
| + global _simSerialCounter | ||
| + serial = 'ed752c5f-f723-437e-bc6c-' + ('%012d' % _simSerialCounter) |
martinpitt
Jun 5, 2016
Owner
This this serial magic in any way? I. e. does ofono do something special for this prefix? If so, please add a comment. If it's just a random number, maybe 12345..abcd? (but that's just a nitpick)
martinpitt
reviewed
Jun 5, 2016
| + | ||
| +def newImsi(): | ||
| + global _imsiCounter | ||
| + imsi = '310150' + ('%09d' % _imsiCounter) |
martinpitt
reviewed
Jun 5, 2016
| @@ -79,6 +94,7 @@ def AddModem(self, name, properties): | ||
| 'Manufacturer': dbus.String('Fakesys', variant_level=1), | ||
| 'Model': dbus.String('Mock Modem', variant_level=1), | ||
| 'Revision': dbus.String('0815.42', variant_level=1), | ||
| + 'Serial': dbus.String(newSimSerial(), variant_level=1), |
martinpitt
Jun 5, 2016
Owner
So newSimSerial() is only being called once, ever. Why the overhead with the function and counter instead of just setting it to a static value and letting the test case drive/set it? Mocks shouldn't be doing unnecessary things by themselves, especially not if the "real thing" doesn't do the same. And I doubt that the serial number changes?
martinpitt
reviewed
Jun 5, 2016
| 'Present': _parameters.get('Present', dbus.Boolean(True)), | ||
| 'Retries': _parameters.get('Retries', dbus.Dictionary([["pin", dbus.Byte(3)], ["puk", dbus.Byte(10)]])), | ||
| 'PinRequired': _parameters.get('PinRequired', "none"), | ||
| 'SubscriberNumbers': _parameters.get('SubscriberNumbers', ['123456789', '234567890']), | ||
| - 'SubscriberIdentity': _parameters.get('SubscriberIdentity', "23456"), | ||
| + 'SubscriberIdentity': _parameters.get('SubscriberIdentity', newImsi()), |
martinpitt
added
the
reviewed/needs-fixing
label
Jun 5, 2016
|
Okay, I've tried to address all your comments.
|
martinpitt
reviewed
Jun 6, 2016
| +# Use a counter so that the result is predictable for tests. | ||
| +def new_modem_serial(mock): | ||
| + serial = '12345678-1234-1234-1234-' + ('%012d' % mock.modem_serial_counter) | ||
| + mock.modem_serial_counter = mock.modem_serial_counter + 1 |
martinpitt
Jun 6, 2016
Owner
+= 1 please, to avoid the repetition of the long var, and easier to read.
martinpitt
reviewed
Jun 6, 2016
| +# Use a counter so that the result is predictable for tests. | ||
| +def new_imsi(mock): | ||
| + imsi = '310150' + ('%09d' % mock.imsi_counter) | ||
| + mock.imsi_counter = mock.imsi_counter + 1 |
martinpitt
reviewed
Jun 6, 2016
| @@ -11,7 +11,7 @@ | ||
| __email__ = 'martin.pitt@ubuntu.com' | ||
| __copyright__ = '(c) 2012 Canonical Ltd.' | ||
| __license__ = 'LGPL 3+' | ||
| -__version__ = '0.16.3' | ||
| +__version__ = '0.16.4' |
martinpitt
Jun 6, 2016
Owner
This gets done automatically on release, commits shouldn't touch this. Please revert.
|
Very nice, thanks! Only three nitpicks, but I'm happy to do them myself during merging, unless you beat me to them. Will merge/test/release tomorrow morning (sorry, running late today). |
martinpitt
added
reviewed/good-with-nitpicks
and removed
reviewed/needs-fixing
labels
Jun 6, 2016
|
Merged with the above nitpick changes. Thanks! |
pete-woods commentedJun 2, 2016
No description provided.