On Sat 16 Feb 2008 at 11:59PM, Shawn Walker wrote:
> 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.
--
> I suspect:
> 96 self.nullf = file("NUL", "w")
--
> There are several lines that print something that are commented out, such as:
> 54 #print "\n------"
> 62 #print "%-5s" % res, d
--
> http://cr.opensolaris.org/~dp/ips-tests/src/tests/api/t_smf.py.html
> 214 # XXX not sure what the desire behavior is here.
--
Shawn, thanks for these comments. Although it is not apparent
from the review (webrev still needs work for Hg, I guess), these
tests/api/t_* files are not new, they are simply relocated from
the tests/t_* files. I'll do the cleanups you suggested, but in
general, they're out of scope.
--
> 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)
Good catch (although it's harmless). Fixed.
> 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?
Thanks; fixed.
> In:
> http://cr.opensolaris.org/~dp/ips-tests/src/tests/cli/testutils.py.html
>
> 163 # print "Nuking: %s" % self.__img_path
>
> Commented debugging statement?
Nuked.
> 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.
I cleaned the logging and temp file stuff up, in general.
I've consolidated all of the testing stuff into /tmp/ips.test.$$.
/tmp/ips.test.$$/depot/ [depot data]
/tmp/ips.test.$$/depot_logdir/$TEST_ID [logging from depotd]
/tmp/ips.test.$$/image
t_depotcontroller is a bit of a special case: It's a test for a piece
of the test harness itself.
> Otherwise, I'm excited to see this change for the test suite; this is
> much closer to what I'm used to seeing.
Great, thanks. I'm hoping to have this in by the end of the week.
-dp
--
Daniel Price - Solaris Kernel Engineering - [EMAIL PROTECTED] - blogs.sun.com/dp
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
