Hi Sue,
I've added docstrings to those items and will push shortly. Thanks for
the reminder.
In general, the tests won't be able to adhere to PEP8 and pylint as
closely as actual code. For example, it's acceptable and even necessary
as part of testing to access non-public variables (i.e., variables whose
names start with a leading underscore), but our current pylintrc file
will complain about that. On the other hand, it's still important to add
docstrings to tests, and to keep under 80 lines, and so on, for
legibility's sake. I'll make a note that we'll want a "test.pylintrc"
file of some kind with looser restrictions for use against test code.
- Keith
On 05/14/10 01:19 PM, Sue Sohn wrote:
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
