Hi Jack, Looks much better - and more likely to behave as you wanted :)
The only question I have now is w.r.t. the tests themselves - you say you did manual testing for expected fails and expected passes, is it possible that we could automate for the fail as part of this too? Thanks, Darren. On 27/06/2012 01: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
