[Re-send - first attempt didn't make it to c-d]
Hi Dermot,
Responses below (removed anything I have no further comments on).
Before you see the responses on the tests, I want to note that for this
specific module and set of Python files, the testing layout should be
sufficient. If it doesn't make sense (from an "effective use of time"
standpoint) to re-organize the tests, I don't think it's strictly
necessary, but I wanted to make the points for future reference and as
an explanation of why I made the initial comments.
- Keith
On 10/ 5/10 10:06 AM, Dermot McCluskey wrote:
Keith,
Many thanks for reviewing so thoroughly.
An updated webrev addressing these issues is at:
http://cr.opensolaris.org/~dermot/webrev_mp_mw_02/
Responses below.
On 10/04/10 22:38, Keith Mitchell wrote:
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?
I've pulled the system-library-install manifest changes from the CUD
gate into
my updated webrev.
They will all go in solaris_install/manifest. The layout will be:
vendor-packages/solaris_install/manifest/
vendor-packages/solaris_install/manifest/__init__.py
vendor-packages/solaris_install/manifest/parser.py
vendor-packages/solaris_install/manifest/writer.py
import statements would look like:
from solaris_install.manifest import ManifestError
from solaris_install.manifest.parser import ManifestParser
from solaris_install.manifest.writer import ManifestWriter
(although if you're using the InstallEngine, you won't need to
import much.)
Thanks for clarifying! That layout makes sense.
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)
I've added the traceback from the original exception to the attributes
of ManifestError:
self.orig_traceback = sys.exc_info()[2]
I haven't added it to the str() output, but I've tested that it can be
accessed from
user code with something like:
import traceback
print("Traceback: %s" % traceback.format_tb(error.orig_traceback))
Is that sufficient?
I think that should be good.
[...]
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)
I'd rather not. MP/MW are really meant to be closely coupled with CUD
and, in
particular, the DOC. We talked previously to Ethan, for example,
about how
other components that don't use the Engine/DOC/etc can very easily
parse XML
documents on their own using lxml, without using MP.
If we want MP to be a general purpose XML handler, then we should
probably
step back and re-look at the overall design to see what functionality
is needed.
That makes sense.
[...]
137, 201, 211, 221, 227, 251, 291: Update logging statement
I don't believe 221 and 227 need to be changed because:
- they are calls to LOGGER.error, not LOGGER.debug and we should
always want to write out errors
- the msg string is used twice, so it definitely needs to be evaluated,
even if we are not logging it
All others fixed.
Works for me.
[...]
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)
D'oh! Thanks.
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.
Agreed. But I think of this module as a bit of an integration test -
testing that
MP works as expected in relation to the InstallEngine and DOC. Also, I
tend to think of use via the InstallEngine as the "default" mode of MP
and
the non-Engine APIs as "add-on" functionality. So, I've put most
tests in the
..._with_engine modules, unless it happened to be easier to check the
outcome
in the ...without_engine module.
I see what you're getting at, but I think that when it comes to unit
testing, really the goal is to isolate the logic tested to the smallest
'unit' possible. So when testing specific features of parse(), it's
better to test them by calling parse() directly. *Unit* tests for
execute() should verify the logic in the execute() function, not verify
the logic in the parse() function - there's no need, since the parse()
function is already verified by its own tests.
For this specific case, the reality is that you're correct, it makes
very little difference - execute() only calls a single function,
parse(), and there's a pretty direct mapping of the inputs to execute()
to the inputs of parse(). However, if there were any logic added to
execute() - say down the road, for some reason, we need to call a
"pre_parse()" method for some reason - now all the 'execute' tests that
are actually testing 'parse()' are complicated by the fact that you have
to workaround the pre_parse() logic and whatever side effects that has
to the input to parse().
[...]
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.
_load_manifest is not a public API, it is only a convenience function.
So, if I can test the code without directly calling this function, should
I not stick to testing the APIs?
For reasons similar to the above on execute()/parse(), it really is
better to have tests against _load_manifest() call the function
directly, rather than indirectly via other APIs, so that as the code
evolves in the future, the tests are maintainable (no one wants to spend
a lot of time maintaining test code) and accurate (if someone fixes a
bug by changing _load_manifest, we want to see the _load_manifest tests
break, not the parse/write/execute tests break).
btw, I should have mentioned that the coverage figures I'm getting from
running my tests are:
% ./slim_test lib/install_manifest/test/ --with-cover
...
Name Stmts Exec
Cover Missing
---------------------------------------------------------------------------------
...
solaris_install.manifest 30 30 100%
solaris_install.manifest.parser 78 78 100%
solaris_install.manifest.writer 89 88
98% 198
...
solaris_install/manifest/parser 78 78 100%
solaris_install/manifest/writer 89 88
98% 198
(although I have a feeling these are slightly exaggerated.)
It's good that the coverage scores aren't indicating any major missing
areas for tests! Thanks for the work in this area.
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)
I would have thought that in general, more tests are better, and
there's no
need to to attempt to consolidate into fewer unit tests.
Is that not the case?
In general, more is better. Certainly, there's no harm in leaving these
in, and it's not necessary to combine. My goal was more to point out
these cases out so that in the future, the time spent creating duplicate
tests could be skipped.
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.
Yes - it is definitely tested, for both positive and negative results,
but via calls
to parse() and/or execute_checkpoints(), not by calling it directly.
That's what it seemed. Thanks!
Thanks,
- Dermot
_______________________________________________
caiman-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/caiman-discuss
