Dan,
Thanks a lot for valuable comments, new webrev is at:
http://cr.opensolaris.org/~migi/automated_tests_16_jul_v1
My answers are inline.
Dan Price wrote:
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.
Because it's additional package it's IMHO much nicer to tell developer
what't need to be done
then just print stack trace. I have changed this to SUNWldtp package.
-- 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.
I have changed this to import ldtp (and removed unused imports). The
pylint was complaining in many
other places as I didn't pylinted the code. Currently all the files have
10.00/10 score. Thanks for reminding me.
-- Since some time has passed by, have you made any progress getting
ldtp into the WOS?
No, I didn't. I thing it can go through ips gate, but if you thing it's
much better to include in the WOS, I will start the
process as the legal is already done.
-- I wasn't sure why you had
39 persistent_depot = True
Those tests are very simple and once those are in the gate we are
planning to extend them. So in many cases
it will be required to have persistent_depot. Currently it's not used. I
have set this to False for those tests that
I am pretty sure won't be changed/extended in the nearest future.
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.
As I did wrote those tests will be extended. I have merged two simple
dialog tests into one, but the rest will contain
more use cases.
-- 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.
deleted...
-- 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()
deleted...
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.
The purpose of doing this way was to easilly increase number to X rows
(let's say 1000 or 10000) to check if the PM
is behaving correctly when loading such big list. Once I will integrate
performance scripts on top of the current framework this would
give us easy way of checking where we are spending time during
startup/switching between categories, etc...
At the moment I left only one package.
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.
I did explain this in the code.
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.
That was a surprise for me! I have checked and main gtk loop doesn't
need to be running to determine if one widget have accessible childrens
in it. I have simplified the code, removed one file and no subprocess
is necessary now. Thanks.
--
best
Michal Pryc
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
