On Wed 15 Jul 2009 at 12:36PM, Michal Pryc wrote:
> Hello pkg-discuss,
>
> I have integrated few new tests that I've got from Conny (CC-ed)
>
> The webrev is at:
> http://cr.opensolaris.org/~migi/automated_tests_15_jul/
Michal,
This looks quite a bit better to me than the first cut. Thank
you for being willing to rework this.
Here are a few things which apply to all the tests:
-- I'm not certain you need the try: except: logic around importing
ldtp in every test-- or perhaps, if you do want to keep it, it
should be clearer about the exact pkg name that the user should
install.
-- As for importing "*"-- I thought that this was somewhat frowned
upon, since it tends to pollute the namespace. Have you tried
running pylint at all? One strategy would be to import ldtp as
ldtp, etc.
-- Since some time has passed by, have you made any progress getting
ldtp into the WOS?
-- I wasn't sure why you had
39 persistent_depot = True
When there is only one test case per file. See next point:
-- It looks like you could potentially subclass
SingleDepotTestCase-->SimpleGuiTestCase -- and you could do most of the setup
and loading of packages into the depot there. This would eliminate about 20
lines of code from each test case. Or you could merge some of your test cases
into one file-- That would probably make things go faster, since you could
take advantage of the persistent depot support.
-- I still see a lot of code commented out in various files-- without
even a comment indicating why. Delete, instead? Perhaps leave behind
some "XXX should do <this> in the future" if you must.
-- This comment is repeated over and over in different files, but could probably
just be omitted:
58 # image_create destroys the previous image
59 # so it's not needed to call self.image_destroy()
Here are some specific comments:
t_pkg_gui_start.py: It seems like you could write this class
as:
template = """
open %[email protected],5.11-0
add set name="description" value="Some package description"
close """
And then in testStartPackagemanager:
for p in ["foo", "bar", "baz", "quux"]:
pkg = template % p
self.pkgsend_bulk(durl, pkg)
Which seems to me less confusing and is less code. On the other
hand, is it really important to have four packages here? It seems
like one would be OK.
check_if_a11y_enabled doesn't need to open pipes to the subprocess,
or read its output does it? It seems like all you need to do is wait()
for it and get the retcode. There should be a comment in
check_if_a11y_enabled() which explains why we need a subprocess.
check_for_a11y: Exit code for "no a11y" should be 1, not 2.
In check_for_a11y, why not exit immediately when you find out
that the a11y is or is not present? Seems like you don't
even need to run the gtk main loop.
-dp
--
Daniel Price, Solaris Kernel Engineering http://blogs.sun.com/dp
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
