-
Notifications
You must be signed in to change notification settings - Fork 295
CP-24132: unit test for usb_scan.py #3324
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
add unit test for usb_scan.py Signed-off-by: Xin(Talons) Li <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there are a few needed changes, one of which requires a minor refactoring in the usb_scan.py script
|
|
||
|
|
||
| @nottest | ||
| def test_log(m): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would avoid using a file, instead I'd mock the stdout or the stderr and use print.
An example is
>>> from StringIO import StringIO
>>> def foo():
... print 'Something'
...
>>> @patch('sys.stdout', new_callable=StringIO)
... def test(mock_stdout):
... foo()
... assert mock_stdout.getvalue() == 'Something\n'
...
>>> test()from the mock documentation at http://www.voidspace.org.uk/python/mock/patch.html
However it doesn't seem that you are using the logs content at all. In that case just replace the log funciton with a void function
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. remove log file, and make test_log empty.
scripts/test_usb_scan.py
Outdated
| @nottest | ||
| def test_usb_exit(self, moc_devices, moc_interfaces, moc_results, | ||
| path="./scripts/usb-policy.conf"): | ||
| with self.assertRaises(SystemExit): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think here you should also check that the failure did show the correct log error, at least the immutable part of it.
You could use @raises(SystemExit) as attribute and assert over the initial part of the error message for example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK.
@nottest def test_usb_exit(self, devices, interfaces, results, path="./scripts/usb-policy.conf", msg=""): with self.assertRaises(SystemExit) as cm: self.test_usb_common(devices, interfaces, results, path) if msg: self.assertIn(msg, cm.exception.message)
scripts/test_usb_scan.py
Outdated
|
|
||
| devices, interfaces = usb_scan.get_usb_info() | ||
|
|
||
| # debug info |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should not copy paste the code, instead we should refactor usb_scan.py to be better suited for testing.
I think we need to extract
Lines 627 to 637 in 3bb7471
| # match interface to device | |
| for i in interfaces: | |
| for d in devices: | |
| if i.is_child_of(d): | |
| d.add_interface(i) | |
| log_list(devices) | |
| # do policy check | |
| policy = Policy() | |
| pusbs = [to_pusb(d) for d in devices if d.is_ready() and policy.check(d)] |
devices and interfaces as parameters and returns the object that we will serialise to json. Then this test should call that function with the mocked devices and interfaces
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK. This will change the usb_scan.py, add a new function do_check(devices, interfaces).
I have to run more test next week.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, call it something like make_pusbs_list(interfaces, devices) and the unit test that function
scripts/test_usb_scan.py
Outdated
| f.write(content) | ||
| self.test_usb_exit([], [], [], path) | ||
|
|
||
| def test_usb_config_error_01(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just a stylistic thing. Here and below, give meaningful names to the error functions. For example, in this case, test_usb_config_error_unexpected_characters and in the next one test_usb_config_error_duplicated_key, and so on
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
scripts/usb_scan.py
Outdated
| :return:([pusb]) the pusb list that can be passed through | ||
| """ | ||
| # debug info | ||
| log_list(devices) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please move these debug info logs to the main body before L648, or remove them completely
1. refactor usb_scan.py for testing 2. make test_log dummy 3. check sys.exit message 4. rename test functions to meaningful names Signed-off-by: Xin(Talons) Li <[email protected]>
add unit test for usb_scan.py
Signed-off-by: Xin(Talons) Li [email protected]