Hi Keith,
Looking at the updated webrev, in disk_selection, you should also add comments
to determine_minimum, determine_recommended, and get_size_line so that pylint
doesn't complain about missing docstrings.
Speaking of pylint - should unit tests (in general) adhere to our PEP8 and
pylint guidelines? If so, your test file might need a bit of attention.
Sue
On 05/13/10 16:45, Keith Mitchell wrote:
Hi John,
Thank you for reviewing. An updated webrev is here:
http://cr.opensolaris.org/~kemitche/14442.e/
On 05/13/10 02:54 PM, John Fischer wrote:
Keith,
You need to update several copyright lines.
Of course I do. I'd assumed I'd already done so since I completed the
fix several weeks ago - but of course, the copyright rules changed in
between...
From disk_selection:
o might want to add a comment to find_size_data().
o since find_size_data() has already updated self.size_line why call
find_size_data() again at 233?
That was a bit awkward. I refactored a bit to use properties to access
DiskScreen.size_line, recommended_size, and minimum_size. It should look
more natural now.
Not a stinkin clue about the test code. I definitely need that
TOI.
These tests are perhaps a little uglier than they might be if this
weren't UI code, and uglier than it would be if it were written with
tests from day 1. That said, the content of each test is not terribly
complex, when broken down. In general, they're composed of three parts:
(1) preparation (2) call the function (3) make assertions on the result
of the function. Simplest case to look at would be
test_determine_size_data: the first two lines ensure the pre-conditions
for the test, the third line calls the function, and the last two lines
ensure that the result is correct based on the intended API.
- Keith
Thanks,
John
On 05/13/10 12:04 PM, Keith Mitchell wrote:
Hi all,
Can I get a code review for the following bug:
http://defect.opensolaris.org/bz/show_bug.cgi?id=14442
Webrev:
http://cr.opensolaris.org/~kemitche/14442_with_tests/
Testing summary:
- Added tests to verify that the disk information is copied only if
there was no previously selected disk, or if the selected disk had
changed
- Added tests to verify that the flag ("do_copy") is set when the
selected disk is changed.
- Refactored DiskScreen.__init__ to pull out code depending on the
image_info file into a separate function. Added tests ensuring that
function works as expected. (When that test is run, there is some
output from the underlying function call due to the missing
image_info file - but the function works regardless).
- Take note of the "MockAll" class I added to the test_disk_select.py
file - it uses some Python tricks and results in an object that can
fulfill a large number of dependencies. Could end up being rather
re-usable in the future.
Beyond the testing, also sanity checked the Text Installer UI to
ensure overall correctness of the solution (this was done previously
a few weeks ago, and re-verified today after merging)
Thanks,
Keith
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
