Hi Jack,
I reviewed the updated webrev:
https://cr.opensolaris.org/action/browse/caiman/schwartz/7177531_2
I have a few comments:
1) You imported logging, and printed log messages as logging.debug().
However, I do not see any code that configures logging settings or
registers log handlers. So, what happens to those log messages?
Why do you choose to use logging instead of print statements?
2) compare_text() and compare_xml() are defined as class functions.
It's not being used elsewhere. Why do they need to be class functions?
3) It would be useful to put comments for the compare_text()
and compare_xml() functions so the reader is clear about what
they do, and what kind of values they return.
4) line 168: Commented line
Thanks,
--Karen
On 06/26/12 17:14, Jack Schwartz wrote:
Hi Darren.
Thanks for reviewing.
On 06/26/12 03:22 AM, Darren Kenny wrote:
Hi Jack,
I'm not sure about the use of grep here - it could potentially fail
should
the DOCTYPE be over two lines.
I know that 'normally' in our XMLs, the DOCTYPE usually will be a single
line, e.g.:
<!DOCTYPE auto_install SYSTEM "file:///usr/share/install/ai.dtd.1">
But it is certainly not unusual, or unsupported, for the DOCTYPE to
be over
multiple lines, e.g.:
<!DOCTYPE auto_install SYSTEM
"file:///usr/share/install/ai.dtd.1">
Which is valid XML.
Also, it certainly doesn't have to be at the start of the line (^) as
the
regular expression assumes.
It might be better to create an XML compare method that loads both
the XML
files and compares recursively...
Redone. New webrev is at:
https://cr.opensolaris.org/action/browse/caiman/schwartz/7177531_2
Retested with existing ai_manifest.xml and DTDs: passes
Retested with Sue's ai_manifest.xml and DTDs in the gate and original
ones on my system: passes
Retested with Sue's ai_manifest and original DTD: fails as it should.
Please bless.
Thanks,
Jack
Thanks,
Darren.
On 25/06/2012 00:43, Jack Schwartz wrote:
Hi everyone.
Please review this unit test fix. Sue found this bug when she changed
the DTD.
https://cr.opensolaris.org/action/browse/caiman/schwartz/7177531_1/webrev.1/
Bug ID: http://monaco.us.oracle.com/detail.jsf?cr=7177531
Bug: unit test that assembles manifest from pieces reads the wrong DTD
The problem was that the manifest was validating against the system's
DTD instead of the DTD in the proto area. (Adding the schema arg to
the
mim constructor fixed that.) The rest of the fix is around performing
the diff of original manifest vs the created one.
Testing:
1) Copied Sue's new DTD and manifest to the proto area. All unit tests
in the MIM overlay module now pass.
2) Restored original DTD and manifest to the proto area. All unit
tests
in the MIM overlay module still pass.
3) pep8 and pylint checked.
Thanks,
Jack
_______________________________________________
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
