On Feb 15, 2008 7:31 PM, Dan Price <[EMAIL PROTECTED]> wrote: > > Hi folks, here's a review (and for those waiting to try it, a proposed > patch) for my rework of the cli testing. > > Not all of the tests are converted. I'll do the rest (two-depots > and memleaks) in another pass. > > For some reason, src/tests/upgrade.ksh is showing up as "changed" > in the webrev. In reality, it is deleted by this wad. So is > src/tests/cli-complete.ksh. > > The t_* api tests have been moved into an "api" subdirectory. > > http://cr.opensolaris.org/~dp/ips-tests/
In several of your src/tests/api/ files, the copyright is 2007 instead of 2008. As for http://cr.opensolaris.org/~dp/ips-tests/src/tests/api/t_catalog.py.html: 95 # XXX How do we do this on Windows? 96 self.nullf = file("/dev/null", "w") See: http://msdn2.microsoft.com/en-us/library/aa293924(VS.60).aspx I suspect: 96 self.nullf = file("NUL", "w") ...will work. In: http://cr.opensolaris.org/~dp/ips-tests/src/tests/api/t_filter.py.html There are several lines that print something that are commented out, such as: 54 #print "\n------" 62 #print "%-5s" % res, d ...etc. Were these temporary debugging statements? In: http://cr.opensolaris.org/~dp/ips-tests/src/tests/api/t_smf.py.html 214 # XXX not sure what the desire behavior is here. s/desire/desired/ In: http://cr.opensolaris.org/~dp/ips-tests/src/tests/cli/t_commandline.py.html 40 self.pkg("image-create --bozo", exit=2) 41 self.pkg("image-create --bozo", exit=2) image-create is listed twice. Is this intentional? In: http://cr.opensolaris.org/~dp/ips-tests/src/tests/cli/t_depot.py.html http://cr.opensolaris.org/~dp/ips-tests/src/tests/cli/t_depotcontroller.py.html http://cr.opensolaris.org/~dp/ips-tests/src/tests/cli/testutils.py.html from time import sleep sleep is imported, but not used? In: http://cr.opensolaris.org/~dp/ips-tests/src/tests/cli/testutils.py.html 163 # print "Nuking: %s" % self.__img_path Commented debugging statement? === General comment: "/tmp/depot.test.%d" This value seems to be used in a lot of places. Any possibility of having as a "constant variable" in unittest and referencing it from there? It would be better, I think, than having the same filename manually typed everywhere. Otherwise, I'm excited to see this change for the test suite; this is much closer to what I'm used to seeing. Cheers, -- Shawn Walker, Software and Systems Analyst http://binarycrusader.blogspot.com/ "To err is human -- and to blame it on a computer is even more so." - Robert Orben _______________________________________________ pkg-discuss mailing list [email protected] http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
