Hi Karen.
On 06/28/12 02:33 PM, Karen Tung wrote:
Hi Jack,
Line 228 on the updated webrev is commented out.
Is that intentional?
No, I meant to remove that comment before pushing. That's how I tested
the various scenarios of stdout/stderr/logging. I'll uncomment before
pushing.
Thanks,
Jack
--Karen
On 06/28/12 13:21, Jack Schwartz wrote:
Hi Karen.
Thanks for reviewing.
On 06/28/12 11:13 AM, Karen Tung wrote:
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?
Logging gets captured by unit tests in a "captured logging" area.
That said, I don't see the output when the file is run standalone.
Not to rathole, but I quickly checked the differences between using
logging, stdout and stderr. Here is my result:
log
standalone
no output
unit test
begin captured logging
stdout
standalone
prints along with dots
unit test
begin captured stdout
stderr
standalone
prints along with dots
unit test
prints along with test numbers
So it appears that stdout is best, as it captures output whether the
test module is run standalone or via slim_test, and the output is
better in the unit test case.
I have changed my output to use stdout.
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?
The question is not why they need to be class functions. The real
question is "why would they need to be instance methods?" They don't
reference any instance variables.
Also, they are not defined outside of the class because only methods
within the class use them.
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.
Done.
4) line 168: Commented line
Deleted.
New webrevs (respun and delta) are here:
https://cr.opensolaris.org/action/browse/caiman/schwartz/7177531_3/webrev.3.2/
Reran pylint and pep8 on the changed file.
Please bless.
Thanks,
Jack
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
