On 09/30/10 07:40 AM, Dermot McCluskey wrote:
Hi All,
I'd like a code review for the ManifestParser/ManifestWriter
checkpoints for the CUD project. The changes are here:
http://cr.opensolaris.org/~dermot/webrev_mp_mw_01/
Please sent comments by 10/12/2010.
Note: this webrev just includes the new files under
usr/src/lib/install_manifest.
There are also a very small number of changes to higher-level
Makefiles, etc
which have been omitted here for convenience.
Hi Dermot,
I can't quite tell from the webrev - will these be under the
solaris_install python package as a sub-package, e.g.,
solaris_install/manifest, solaris_install/manifest/common,
solaris_install/manifest/parser?
Specific comments below.
- Keith
common/__init__.py:
42: I can't recall - will storing the Exception preserve the original
traceback as well? I seem to remember thinking that the traceback must
be explicitly grabbed (via, e.g., the traceback module) if desired for
later. It may be worth adding that instrumentation to this class (or,
logging the traceback may be sufficient - see below on logging.exception)
62: As this function is intended for use by modules other than the file
it's defined in, I think it's more appropriate to define without the
leading underscore.
87: In general, it's better to use logger.exception in 'except' clauses
- this will automatically log things like traceback data, etc.
(Obviously, it depends on the scenario - sometimes it's not needed - but
in general, it's better to have the traceback data than to end up
wanting it later)
parser.py:
166-7: Why not just "if validate_from_docinfo not in [None, True,
False]:"? Although I'm generally of the opinion that there's no need to
explicitly verify that a parameter is a bool or not. Just check for "is
(not) None" at the appropriate point, then run "if
validate_from_docinfo" at the appropriate point. Type-checking in Python
(particularly for bools) shouldn't be needed in most cases.
178-186: Same comment regarding type-checking of bools.
231: While most of our code is probably not terribly performance
sensitive when it comes to formatting string operations, it's best
practice, when making logging statements, to use the form:
LOGGER.debug("ManifestParser.parse(doc=%s) called", doc)
(Note that doc is a separate arg, and not cast to a string first, rather
than pre-formatted using %)
This has the advantage of, if LOGGER is not enabled for the debug level,
the string formatting doesn't occur. (Note that the call to str(doc) is
unnecessary regardless - '%s' will call str() on the passed in object)
235: I don't think this should be strictly needed - just pass in
_validate_from_docinfo. "None" evaluates to "False" in a boolean
context. Only reason to cast this would be if the dtd_validation arg to
etree.XMLParser has differing behavior for None vs. False - and if
that's the case, just pass bool(_validate_from_docinfo) to the
_load_manifest() call, no need for the separate if/else block here.
270, 338, 345: Use logging.exception
273: Would it make sense to have parse() return a reference to the xml
tree? This wouldn't change the execute behavior, and would allow for
parse() to have some additional value when it doesn't get a DOC
parameter. (Mostly useful for diagnostics, probably)
295: See previous comment on string formatting for logging operations.
351: Along the same lines as the string formatting for logging comment,
logging statements like this should be enclosed in 'if' blocks like so:
if LOGGER.isEnabledFor(logging.DEBUG):
LOGGER.debug(some_function_with_potentially_long_execution_time())
Again, it probably isn't much of a strain here, but it's good practice
to try to isolate it.
writer.py:
29-32, 34-37: Nit: Alphabetize imports
137, 201, 211, 221, 227, 251, 291: Update logging statement
149: See previous comment about type-checking bools
264: This will error out if the open() call on 256 failed.
Test code:
Note: I skipped over the test XML/DTD/XSLT files, unless they need
explicit reviewing, focusing on the actual test code. Also, much as I
love test code, I find it hard to review (sometimes difficult to
correlate test cases with tested code if you're not directly working on
either - this isn't a problem with your code, just with my eyes), so if
I comment on a "missing" test case that exists, just let me know.
test/common.py:
78-97: lists in Python support negative indices - so this could be
condensed to:
try:
line = lines[lineno].strip()
except IndexError:
return False
else:
return (line == string)
test_mp_with_engine.py:
General: This module, in particular, doesn't need much (if anything at
all). There's no strict logic in the execute() method other than the doc
verification, so unit tests don't add significant value beyond what's
more readily testable specifically against the parse() method.
51: Should be done in tearDown(), so that test modules that run *after*
this one can sanely instantiate the Engine if needed. (Additionally, I
think somewhere in CUD there is an engine reset function for use in test
code - use that. I believe it also resets the loggerDict)
78, 100, 137, 153, 171: Use self.assertEqual(status,
InstallEngine.EXEC_SUCCESS, message)
84: Use assertEqual (better default msg on failure)
test_mp_without_engine.py:
Missing: Tests that run _load_manifest() directly (if some of the
existing tests hit that logic specifically, they should be written to
call _load_manifest directly, rather than via parse()). Only a few test
cases are needed - specifically, maybe 1 success case with xinclude, 1
with dtd_validation=True, 1 with attribute_defaults=True; 1 error case
where an IOError -> ManifestError occurred, and 1 where XMLSyntaxError
-> ManifestError occurred.
test_mw_with_engine.py: Similar comment to test_mp_with_engine. Some of
these tests seem to duplicate those in test_mw_without_engine. The two
relevant cases unique to the execute() function itself are the dry_run
flag, and the "doc is None" error condition. (This file could be readily
combined with the dry_run test file)
Other:
is _validate_manifest tested directly anywhere? It appears that some of
the writer/parser tests might be covering symptoms of _validate_manifest
that would be better served by testing the function directly.
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
