Danek Duvall wrote:
On Wed, Jul 15, 2009 at 12:36:24PM +0200, Michal Pryc wrote:
http://cr.opensolaris.org/~migi/automated_tests_15_jul/
This does look a whole lot better; thanks.
Thanks for reviewing it. My comments are inline and the new webrev:
http://cr.opensolaris.org/~migi/automated_tests_16_jul_v1
Could you talk a bit about what the test naming scheme is? I don't think
that either "pkg" or "gui" is necessary in the filenames. "Gui" is
implicit by the fact that we're in the gui directory, and "pkg" is there
for the cli tests because not all the CLIs test the "pkg" program. So I'd
expect "t_pm_XXX.py" and "t_um_XXX.py" and so on. However, I don't
understand what the ones without "_pm_" are, except possibly misnamed.
I have changed this, so it's now what you proposed:
t_pm_XXX.py
Currently we don't have UM or Webinstall tests, but if we will, then the
naming convension will be:
t_um_*
t_wi_*
__init__.py:
- line 23: copyright year should be 2009
Changed.
check_for_a11y.py:
- line 26: unused
- line 29: is it necessary to run code here instead of in, the __main__
section, or check_if_a11y_enabled()?
- line 52: we're using exit code 2 as a usage error, not general failure;
1 is more appropriate here.
- line 57: don't need an explicit return here; you don't use it, anyway.
I have removed this file, as starting the gtk.main() is not necessary to
determine if a11y is enabled. Currently everything is much more
simplified and accessed through the testguiutils.py.
t_pkg_gui_addrepo.py:
- line 35: Is this really necessary? It's barely more information than
what you'd get with the bare import statements.
As Dan advised I have chandeg this to inform what package is needed.
- line 63: This is pretty common; perhaps this should be in common code.
You could put it into the gui module, but it'd be fine somewhere in
cli.testutils, too.
I left this here as I don't want to do "import ldtp" in the cli.testutils.
- line 75: add a space after the comment character
changed
t_pkg_gui_install.py:
- line 77: objectexist() really returns an integer instead of a boolean?
Is there no way to get notification of this event, rather than polling?
Not that I am aware of using ldtp. There are other waitforwindow
methods, but there isn't any for widgets.
- line 80: "focus"
- line 89: no space between function name and parenthesis
- line 90: remove trailing newline
all three changed (I did pylint this time so it's 10.0/10)
t_pkg_gui_pm_about.py:
- Do we really need a test for the about box?
Yes. All windows needs to be tested as sometimes they were not working
due to different bugs. We could think about skipping some of the tests
in the general test?
- If we do, do we really need to start up a depo, send it a package, and
create an image?
Yes, without doing that we would either load packages from the default
image, which takes time... or the PM will print error dialog telling
that package list is empty, so it's different use case.
t_pkg_gui_pm_help.py:
- same questions apply for help as for about
explained above.
- line 72: don't check in commented-out code.
removed.
t_pkg_gui_remove.py:
- line 74: "focus"
changed
t_pkg_gui_start.py:
I have to question the value of this test -- you test this functionality
in all the other tests.
testguiutils.py:
- Is there a reason you run the check as a separate process instead of
inline?
it's inline now.
- line 30: no spaces around equals signs in argument lists.
not needed, as we now don't call subprocess.
- line 35: this should probably return True or False
changed.
--
best
Michal
_______________________________________________
pkg-discuss mailing list
[email protected]
http://mail.opensolaris.org/mailman/listinfo/pkg-discuss
