-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard-openlmi.rhcloud.com/r/1147/#review1566
-----------------------------------------------------------
src/software/test/testSoftware.py
<
http://reviewboard-openlmi.rhcloud.com/r/1147/#comment959>
Use 'cls' instead of 'self' in classmethods. See pep8:
http://www.python.org/dev/peps/pep-0008/#function-and-method-arguments
src/software/test/testSoftware.py
<
http://reviewboard-openlmi.rhcloud.com/r/1147/#comment960>
What is the purpose of this function? Remove epoch from NVR? If so then note that
epoch could be arbitrary number, this works only for epoch <0..9>.
src/software/test/testSoftware.py
<
http://reviewboard-openlmi.rhcloud.com/r/1147/#comment961>
I would rather use endswith:
for arch in ("x86_64", "noarch", "amd64",
"i686", "i386"):
if word.endswith(arch):
...
But that's not really an issue.
src/software/test/testSoftware.py
<
http://reviewboard-openlmi.rhcloud.com/r/1147/#comment962>
use "assertListEqual", it produces better output when the test fails
src/software/test/testSoftware.py
<
http://reviewboard-openlmi.rhcloud.com/r/1147/#comment963>
The matching against 'yum list available' is missing. If it is on purpose, add
at least a TODO comment.
src/software/test/testSoftware.py
<
http://reviewboard-openlmi.rhcloud.com/r/1147/#comment964>
assertEqual -> assertListEqual
src/software/test/testSoftware.py
<
http://reviewboard-openlmi.rhcloud.com/r/1147/#comment965>
You can use
.InstalledSoftware.InstanceID
instead of
.path["InstalledSoftware"]["InstanceID"]
src/software/test/testSoftware.py
<
http://reviewboard-openlmi.rhcloud.com/r/1147/#comment966>
typo: rise -> raise (occurs multiple times)
src/software/test/testSoftware.py
<
http://reviewboard-openlmi.rhcloud.com/r/1147/#comment967>
Don't use PG_ComputerSystem directly, there should be LMI_CS_CLASSNAME env var,
see
https://fedorahosted.org/openlmi/ticket/161
src/software/test/testSoftware.py
<
http://reviewboard-openlmi.rhcloud.com/r/1147/#comment968>
self.assertIsNone()
The test suite looks good to me in general. I don't have any objections against
merging it into upstream repo, but I would like to let Michal decide if he is satisfied
with the test suite this way.
- Radek Novacek
On Oct. 30, 2013, 3:03 p.m., Jan Grec wrote:
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard-openlmi.rhcloud.com/r/1147/
-----------------------------------------------------------
(Updated Oct. 30, 2013, 3:03 p.m.)
Review request for OpenLMI Developers.
Repository: openlmi-providers
Description
-------
software - tests: software test suite with basic tests
This is only a "current-status" review. I'll be very thankful for any
comments on what's wrong, how the structure should look like, etc.
My TODO is to
* repair all broken tests
* add all tests mentioned
* bind test suite with new LmiTestCase class
I apologize for inconvenience, but I wasn't able to save previous review notes.
Diffs
-----
src/software/test/lmi-test.repo PRE-CREATION
src/software/test/testSoftware.py PRE-CREATION
Diff:
http://reviewboard-openlmi.rhcloud.com/r/1147/diff/
Testing
-------
Thanks,
Jan Grec