Keith,
Responses below.
An updated webrev is at: http://cr.opensolaris.org/~dermot/webrev_mp_mw_03/
On 10/05/10 20:08, Keith Mitchell wrote:
[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).
I accept your point. Some of my tests are clearly more integration
tests than
proper unit tests. I will bear this in mind in future.
I've added a new test module, test_mp_load_manifest.py, that adds the
additional tests you
suggest and have removed a few of the tests from test_mp_with_engine
that were checking
similar functionality by via higher-level API calls.
I have not, however, done a complete reorg of the tests to fit in with
this scheme. I hope
that is sufficient for now?
Thanks,
- Dermot
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
