After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 748006 - Automated test for Fedora 20 express install
Automated test for Fedora 20 express install
Status: RESOLVED FIXED
Product: gnome-boxes
Classification: Applications
Component: tests
3.16.x
Other Linux
: Normal normal
: --
Assigned To: GNOME Boxes maintainer(s)
Vladimir Benes
Depends on: 744852
Blocks: 750701
 
 
Reported: 2015-04-16 19:34 UTC by vladimir benes
Modified: 2016-09-20 08:15 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
tests: Fedora 20 express install (7.25 KB, patch)
2015-04-16 19:47 UTC, vladimir benes
needs-work Details | Review
tests: Add "libvirt_domain_get_vm_state" function (1.46 KB, patch)
2015-06-02 09:24 UTC, vladimir benes
needs-work Details | Review
tests: Add "Installation finished check" step (1.18 KB, patch)
2015-06-02 09:38 UTC, vladimir benes
accepted-commit_now Details | Review
tests: Add "Verify user with password" step (1.66 KB, patch)
2015-06-02 10:56 UTC, vladimir benes
accepted-commit_now Details | Review
tests: URL Install step express install modification (1.50 KB, patch)
2015-06-02 11:28 UTC, vladimir benes
needs-work Details | Review
tests: Remove "Type text" step (6.54 KB, patch)
2015-06-02 13:56 UTC, vladimir benes
needs-work Details | Review
tests: Add libvirt_domain_get_vm_state() (1.44 KB, patch)
2015-06-02 15:00 UTC, vladimir benes
none Details | Review
tests: Handle setup page in create_new_vm_via_url (1.37 KB, patch)
2015-06-02 15:12 UTC, vladimir benes
none Details | Review
tests: Enlarge timeout for media download (976 bytes, patch)
2015-06-02 15:16 UTC, vladimir benes
none Details | Review
tests: Add "Type text" step (944 bytes, patch)
2015-06-02 15:24 UTC, vladimir benes
none Details | Review
tests: Rework "Type text" step (6.49 KB, patch)
2015-06-02 15:29 UTC, vladimir benes
none Details | Review
tests: Add "Express install Fedora 20" test (3.56 KB, patch)
2015-06-03 12:01 UTC, vladimir benes
none Details | Review
tests: Add "Installation finished check" step (1.39 KB, patch)
2015-06-03 12:06 UTC, vladimir benes
none Details | Review
tests: Rework "Type text" step (6.69 KB, patch)
2015-06-03 12:14 UTC, vladimir benes
none Details | Review
tests: Rework "Type text" step (6.53 KB, patch)
2015-06-03 12:17 UTC, vladimir benes
none Details | Review
tests: Add libvirt_domain_get_vm_state() (1.44 KB, patch)
2015-06-03 12:25 UTC, vladimir benes
needs-work Details | Review
tests: Add "Installation finished check" step (1.46 KB, patch)
2015-06-03 12:32 UTC, vladimir benes
accepted-commit_now Details | Review
tests: Add "Verify user with password" step (1.68 KB, patch)
2015-06-03 12:34 UTC, vladimir benes
accepted-commit_now Details | Review
tests: Handle setup page in create_new_vm_via_url (1.37 KB, patch)
2015-06-03 12:39 UTC, vladimir benes
accepted-commit_now Details | Review
tests: Enlarge timeout for media download (976 bytes, patch)
2015-06-03 12:40 UTC, vladimir benes
accepted-commit_now Details | Review
tests: Rework "Type text" step (6.53 KB, patch)
2015-06-03 12:41 UTC, vladimir benes
needs-work Details | Review
tests: Add "Express install Fedora 20" test (3.56 KB, patch)
2015-06-03 12:47 UTC, vladimir benes
needs-work Details | Review
tests: Add "Type text and return" step (1.03 KB, patch)
2015-06-09 12:23 UTC, vladimir benes
none Details | Review
tests: Add libvirt_domain_get_install_state() (1.45 KB, patch)
2015-06-10 11:26 UTC, vladimir benes
committed Details | Review
tests: Add "Installation finished check" step (1.48 KB, patch)
2015-06-10 11:29 UTC, vladimir benes
committed Details | Review
tests: Add "Verify user with password" step (1.68 KB, patch)
2015-06-10 11:31 UTC, vladimir benes
committed Details | Review
tests: Handle setup page in create_new_vm_via_url (1.37 KB, patch)
2015-06-10 11:32 UTC, vladimir benes
committed Details | Review
tests: Increase timeout for media download (978 bytes, patch)
2015-06-10 11:34 UTC, vladimir benes
committed Details | Review
tests: Add "Type text and return" step (1.03 KB, patch)
2015-06-10 11:47 UTC, vladimir benes
committed Details | Review
tests: No implicit return in "Type text" step" (6.56 KB, patch)
2015-06-10 11:55 UTC, vladimir benes
committed Details | Review
tests: Add "Express install Fedora 20" test (3.22 KB, patch)
2015-06-10 12:02 UTC, vladimir benes
none Details | Review
tests: Add "Express install Fedora 20" test (3.19 KB, patch)
2015-06-10 12:45 UTC, vladimir benes
committed Details | Review

Description vladimir benes 2015-04-16 19:34:38 UTC
adding express install bits and first Fedora 20 test. Pexpect dependency added.
Comment 1 vladimir benes 2015-04-16 19:47:44 UTC
Created attachment 301758 [details] [review]
tests: Fedora 20 express install

Add Fedora 20 express install and inftrastructure bits. Test adds user
with password and waits until anaconda installation is over. Check is
performed via os-state tag reported by libvirt. Check whether user is
added correctly is done over ssh. This needs another dependency to
pexpect.
Comment 2 Zeeshan Ali 2015-04-24 15:27:21 UTC
Review of attachment 301758 [details] [review]:

Really happy to see this patch but I feel that it can and should be divided a bit. I would put each new function addition to support exress install feature into its own patch. The README file changes will also needed to be divided (i-e the info of dep addition would go into the patch that introduces the dep).

::: tests/environment.py
@@ +171,3 @@
+       if not os.path.isfile('/usr/lib/python*/site-packages/pexpect/__init__.py'):
+            do_restore()
+            sys.exit(77)

What is going on here?

@@ +207,3 @@
 def after_tag(context, tag):
+    if 'express_install' in tag:
+        call('rm -rf /home/test/.cache/gnome-boxes/unattended', shell=True)

A comment above this line would be nice.

::: tests/steps/creation.py
@@ +56,3 @@
             half_minutes += 1
+            if context.app.findChild(GenericPredicate(\
+                name='Choose express install to automatically preconfigure the box with optimal settings.'),\

missing space after '='.

::: tests/steps/express-install.py
@@ +16,3 @@
+    return value
+
+def libvirt_domain_get_domain_context(dom):

I think you need to sort out the hierarchy of your patches. This function is also added as part of bug#747776. It's perfectly normal to base one patch series over the other but you can't be adding the same changes in two different patches at the same time (i-e one has to be rebased on the other).

@@ +40,3 @@
+
+@step('Check if installation of "{machine}" is finished in "{max_time}" minutes')
+def check_vm_state(context, machine, max_time):

name suggests its not specific to a state but its clearly meant to check for a specific state.

@@ +50,3 @@
+			sleep(60)
+
+	assert state == 'installed', "Machine %s is not in installed but stayed in %s after %s minutes" %(machine, state, max_time)

does this line really fits 120 chars?

@@ +53,3 @@
+
+@step('Verify "{user}" user with "{pwd}" password in "{machine}"')
+def verify_user(context, user, pwd, machine):

I thought of 'print working directory' command when I read 'pwd' and was confused for a bit. Coding style is not to use unknown abbreviations. Let's just name it 'password' or 'passwd'.

@@ +58,3 @@
+	ssh.sendline(pwd)
+	sleep(10)
+	if 'Fedora' in machine:

Why are we doing these non-user checks for Fedora only in this function meant only to verify user? I don't think this is needed.

::: tests/steps/utils.py
@@ +81,3 @@
+@step('Just type "{text}"')
+def justtype_text(context, text):
+    typeText(text)

* The difference with type_text is not obvious from name or signature so I'll rename type_text to 'enter_text' and name this as 'type_text'.

* Although a very small change, I'd put this in a separate patch.
Comment 3 Lasse Schuirmann 2015-04-24 15:50:51 UTC
Review of attachment 301758 [details] [review]:

Hi,

just throwing in my little bits since I've done a lot in python lately. I agree with Zeeshan that this should be split up, solely by noticing how many files this touches at once.

::: tests/environment.py
@@ +207,3 @@
 def after_tag(context, tag):
+    if 'express_install' in tag:
+        call('rm -rf /home/test/.cache/gnome-boxes/unattended', shell=True)

you could use rmtree from python, seems cleaner to me: https://docs.python.org/2/library/shutil.html#shutil.rmtree

::: tests/steps/creation.py
@@ +56,2 @@
             half_minutes += 1
+            if context.app.findChild(GenericPredicate(\

that trailing backslash shouldn't be needed as there are brackets.

@@ +56,3 @@
             half_minutes += 1
+            if context.app.findChild(GenericPredicate(\
+                name='Choose express install to automatically preconfigure the box with optimal settings.'),\

a usual style is for keyword arguments not to have spaces around the '=' in python so this would be the preferred notation to me actually.

Backslash shouldn't be needed here too. I'd adjust the indentation a bit and use implicit string concatenation btw.:
            if context.app.findChild(
                GenericPredicate(name='Choose express install to automatically '
                                      'preconfigure the box with optimal settings.')
                retry=False,
                requireResult=False):

(Copy that in yuour editor to get alignment right.)

@@ +57,3 @@
+            if context.app.findChild(GenericPredicate(\
+                name='Choose express install to automatically preconfigure the box with optimal settings.'),\
+                retry= False, requireResult=False):

space after `retry=` needs to be removed then. If Zeeshan agrees of course.

I'd put each argument in an own line, not only this looks ugly to me but also it is way easier to scan through all the keywords if they are all beginning in the same column.

::: tests/steps/express-install.py
@@ +11,3 @@
+    res = ctx.xpathEval(path)
+    if res is None or len(res) == 0:
+        value="Unknown"

spacing around operator missing

@@ +61,3 @@
+		ssh.sendline('cat /etc/redhat-release')
+		if "Fedora 20" in machine:
+			ssh.expect('Fedora release 20')

at least in this file you use mixed tabs/spaces. I'd actively check for this issue because this is really bad and when we do the python 3 migration (hey python3 is the default implementation on new fedora isn't it?) this will stop working because 3 doesn't allow that. However there are also other reasons for which mixing those is bad I probably don't need to name. Spaces are usually preferred with an indentation of 4.
Comment 4 Zeeshan Ali 2015-04-24 16:15:52 UTC
Review of attachment 301758 [details] [review]:

::: tests/steps/creation.py
@@ +56,3 @@
             half_minutes += 1
+            if context.app.findChild(GenericPredicate(\
+                name='Choose express install to automatically preconfigure the box with optimal settings.'),\

Ah, i missed the fact that they are implicit arguments and not just a simple assignment.

@@ +57,3 @@
+            if context.app.findChild(GenericPredicate(\
+                name='Choose express install to automatically preconfigure the box with optimal settings.'),\
+                retry= False, requireResult=False):

sure.

::: tests/steps/express-install.py
@@ +61,3 @@
+		ssh.sendline('cat /etc/redhat-release')
+		if "Fedora 20" in machine:
+			ssh.expect('Fedora release 20')

Oh I missed this. Wish splinter would should this.
Comment 5 vladimir benes 2015-06-02 09:24:55 UTC
Created attachment 304415 [details] [review]
tests: Add "libvirt_domain_get_vm_state" function

This function can be used for VM state checking from libvirt. It
can say whether system is none, live, installing, or installed.
Comment 6 vladimir benes 2015-06-02 09:38:44 UTC
Created attachment 304417 [details] [review]
tests: Add "Installation finished check" step

Step that asks libvirt every minute if VM state is installed. Step
fails after user specified amount of time if not.
Comment 7 vladimir benes 2015-06-02 10:56:10 UTC
Created attachment 304423 [details] [review]
tests: Add "Verify user with password" step

Check over ssh (using pexpect) that user is able to log into VM with
password.
Comment 8 vladimir benes 2015-06-02 11:28:54 UTC
Created attachment 304424 [details] [review]
tests: URL Install step express install modification

Create new box from url step is now able to handle express install
screen. It waits up to 60 minutes for media download.
Comment 9 Zeeshan Ali 2015-06-02 13:25:57 UTC
Review of attachment 304415 [details] [review]:

* No need for quotes around function (or any identifier names) since they don't include spaces to cause any readability issues, rather put () after it to keep it clear its a function.
* Convention is to use '-' in filenames rather than '_'.
* Splitting this into a separate patch reveals an minor issue: this function doesn't seem to belong to express-install.py. How about adding it to the same place you add other libvirt domain handling functions in other bugs/patches? Its ok to have inter-dependencies between patches of different bugs as long as you setup deps between bugs.
Comment 10 Zeeshan Ali 2015-06-02 13:27:06 UTC
Review of attachment 304417 [details] [review]:

Looks good, apart from the same comment that applies to previous comment about filename: '-' instead of '_'.
Comment 11 Zeeshan Ali 2015-06-02 13:29:36 UTC
Review of attachment 304423 [details] [review]:

Looks good. ACK
Comment 12 Zeeshan Ali 2015-06-02 13:42:05 UTC
Review of attachment 304424 [details] [review]:

* shortlog has to be read many times to understand what you mean. How about "tests: Handle setup page in create_new_vm_via_url()"? It goes beyond 50 chars probably full name of function isn't that important here.
* URL should be in CAPS in description too.
* Why are we changing the waiting time? Doesn't seem related to this change itself.
Comment 13 vladimir benes 2015-06-02 13:45:27 UTC
(In reply to Zeeshan Ali (Khattak) from comment #12)
> Review of attachment 304424 [details] [review] [review]:
> 
> * shortlog has to be read many times to understand what you mean. How about
> "tests: Handle setup page in create_new_vm_via_url()"? It goes beyond 50
> chars probably full name of function isn't that important here.
> * URL should be in CAPS in description too.
> * Why are we changing the waiting time? Doesn't seem related to this change
> itself.

it's described in info, it is needed for media download. 20 minutes is not enough. 60 should always be.
Comment 14 Zeeshan Ali 2015-06-02 13:51:21 UTC
(In reply to vladimir benes from comment #13)
> (In reply to Zeeshan Ali (Khattak) from comment #12)
> > Review of attachment 304424 [details] [review] [review] [review]:
> > 
> > * shortlog has to be read many times to understand what you mean. How about
> > "tests: Handle setup page in create_new_vm_via_url()"? It goes beyond 50
> > chars probably full name of function isn't that important here.
> > * URL should be in CAPS in description too.
> > * Why are we changing the waiting time? Doesn't seem related to this change
> > itself.
> 
> it's described in info, it is needed for media download.

That was the case already (i-e it needed to wait). You are changing the timeout so that needs justification of sorts. :) In any case, I don't think this change belongs to this patch and it'll need its own justification anyway.
Comment 15 vladimir benes 2015-06-02 13:56:26 UTC
Created attachment 304428 [details] [review]
tests: Remove "Type text" step

Type text and return step (powered by xdotool) is used instead so
Type step can now be removed and all needed steps are changed to use
the only way possible.
Comment 16 Zeeshan Ali 2015-06-02 14:07:54 UTC
Review of attachment 304428 [details] [review]:

::: tests/README
@@ -83,3 @@
 
-* for send key combos test execution
-  * xdotool

This is being added in a unmerged patch, only to get removed here. I think you probably want this patch to replace that unmerged patch instead (on the other bug). Alternatively you can do this unification *after* both patch serieses have been merged.
Comment 17 vladimir benes 2015-06-02 15:00:59 UTC
Created attachment 304438 [details] [review]
tests: Add libvirt_domain_get_vm_state()

This function can be used for VM state checking from libvirt. It
can say whether system is installing or installed.
Comment 18 vladimir benes 2015-06-02 15:12:58 UTC
Created attachment 304439 [details] [review]
tests: Handle setup page in create_new_vm_via_url

Create new box from URL step is now able to handle express install
screen.
Comment 19 vladimir benes 2015-06-02 15:16:04 UTC
Created attachment 304440 [details] [review]
tests: Enlarge timeout for media download

Tests are now waiting for media download up to 60 minutes as it's
needed for large installation image downloads like Fedora 20 has.
Comment 20 vladimir benes 2015-06-02 15:19:51 UTC
(In reply to Zeeshan Ali (Khattak) from comment #16)
> Review of attachment 304428 [details] [review] [review]:
> 
> ::: tests/README
> @@ -83,3 @@
>  
> -* for send key combos test execution
> -  * xdotool
> 
> This is being added in a unmerged patch, only to get removed here. I think
> you probably want this patch to replace that unmerged patch instead (on the
> other bug). Alternatively you can do this unification *after* both patch
> serieses have been merged.

It was added to specific key combo section and the patch hopefully merged now. It was removed from specific section and moved all the way up to general requirements as it's now widely used in various tests.
Comment 21 vladimir benes 2015-06-02 15:24:22 UTC
Created attachment 304443 [details] [review]
tests: Add "Type text" step

Add "Type text" step for entering text without pressing return in
the end.
Comment 22 vladimir benes 2015-06-02 15:29:08 UTC
Created attachment 304444 [details] [review]
tests: Rework "Type text" step

Type text and return is used instead of original step and new name
is now used for entering text only (without return). All steps were
changed to use the new style.
Comment 23 vladimir benes 2015-06-03 12:01:38 UTC
Created attachment 304496 [details] [review]
tests: Add "Express install Fedora 20" test

Add test that downloads Fedora 20 4.6 GB iso and starts express
installation from it. Test user with password is added and checked
via ssh if present after installation.
Comment 24 vladimir benes 2015-06-03 12:06:04 UTC
Created attachment 304498 [details] [review]
tests: Add "Installation finished check" step

Step that asks libvirt every minute if VM state is installed. Step
fails after user specified amount of time if not.
Comment 25 vladimir benes 2015-06-03 12:14:51 UTC
Created attachment 304500 [details] [review]
tests: Rework "Type text" step

All original "Type text" steps were ported to "Type text and return"
 step instead as both were doing the same.

New "Type text" step was introduced. This step just writes text but
doesn't do return at the end.

Type text and return is used instead of original step and new name
is now used for entering text only (without return). All steps were
changed to use the new style.
Comment 26 vladimir benes 2015-06-03 12:17:23 UTC
Created attachment 304501 [details] [review]
tests: Rework "Type text" step

All original "Type text" steps were ported to "Type text and return"
 step instead as both were doing the same.

New "Type text" step was introduced. This step just writes text but
doesn't do return at the end.
Comment 27 vladimir benes 2015-06-03 12:23:11 UTC
I have to clean this up a bit.. this got somehow messy.. sorry for that
Comment 28 vladimir benes 2015-06-03 12:25:37 UTC
Created attachment 304502 [details] [review]
tests: Add libvirt_domain_get_vm_state()

This function can be used for VM state checking from libvirt. It
can say whether system is installing or installed.
Comment 29 vladimir benes 2015-06-03 12:32:31 UTC
Created attachment 304504 [details] [review]
tests: Add "Installation finished check" step

Step that asks libvirt every minute if VM state is installed. Step
fails after user specified amount of time if not.
Comment 30 vladimir benes 2015-06-03 12:34:51 UTC
Created attachment 304505 [details] [review]
tests: Add "Verify user with password" step

Check over ssh (using pexpect) that user is able to log into VM with
password.
Comment 31 vladimir benes 2015-06-03 12:39:11 UTC
Created attachment 304506 [details] [review]
tests: Handle setup page in create_new_vm_via_url

Create new box from URL step is now able to handle express install
screen.
Comment 32 vladimir benes 2015-06-03 12:40:24 UTC
Created attachment 304507 [details] [review]
tests: Enlarge timeout for media download

Tests are now waiting for media download up to 60 minutes as it's
needed for large installation image downloads like Fedora 20 has.
Comment 33 vladimir benes 2015-06-03 12:41:48 UTC
Created attachment 304508 [details] [review]
tests: Rework "Type text" step

All original "Type text" steps were ported to "Type text and return"
step instead as both were doing the same.

New "Type text" step was introduced. This step just writes text but
doesn't do return at the end.
Comment 34 vladimir benes 2015-06-03 12:47:09 UTC
Created attachment 304509 [details] [review]
tests: Add "Express install Fedora 20" test

Add test which downloads Fedora 20 4.3 GB iso and starts express
installation with it. Test user with password is added and checked
via ssh if present after installation.
Comment 35 Zeeshan Ali 2015-06-04 16:28:36 UTC
Review of attachment 304502 [details] [review]:

All comments in comment#9 addressed but one:

"* Splitting this into a separate patch reveals an minor issue: this function doesn't seem to belong to express-install.py. How about adding it to the same place you add other libvirt domain handling functions in other bugs/patches? Its ok to have inter-dependencies between patches of different bugs as long as you setup deps between bugs."

You mentioned on IRC that function is related to installation so we can put it under 'installation.py' if you like. While you are at it, you might want to rename it to libvirt_domain_get_install_state() since 'vm' and 'domain' are synonyms anyway and it'd make code more readable.
Comment 36 Zeeshan Ali 2015-06-04 16:29:48 UTC
Review of attachment 304504 [details] [review]:

ack
Comment 37 Zeeshan Ali 2015-06-04 16:30:35 UTC
Review of attachment 304505 [details] [review]:

ack
Comment 38 Zeeshan Ali 2015-06-04 16:32:22 UTC
Review of attachment 304506 [details] [review]:

I'd add () after method name in log even if it goes beyond 50 chars but ACK in any case.
Comment 39 Zeeshan Ali 2015-06-04 16:33:19 UTC
Review of attachment 304507 [details] [review]:

Thanks for splitting. Just one micro nitpick: Enlarge -> Increase.
Comment 40 Zeeshan Ali 2015-06-04 16:43:16 UTC
Review of attachment 304508 [details] [review]:

* "Rework" is way too vague and makes one think this is multiple different changes. How about "tests: No implicit return in "Type text" step"?
* From https://wiki.gnome.org/Git/CommitMessages
  "Use imperative form of verbs rather than past tense when referring to changes introduced by commit in question. For example "Remove property X" rather than "Removed property X"."
* "New "Type text" step was introduced." sounds like new step but this patch doesn't add any new functions. If it did, I'd ask you to split the patch. :)

::: tests/README
@@ -84,3 @@
-* for send key combos test execution
-  * xdotool
-

This change is still here but you didn't answer my question from comment#16.
Comment 41 Zeeshan Ali 2015-06-04 16:45:54 UTC
Review of attachment 304509 [details] [review]:

Great stuff.

::: tests/environment.py
@@ +174,3 @@
+        except:
+            do_restore()
+            sys.exit(77)

Let's have a hard dep on pexpect in too unless there is a good reason not to.

@@ +214,3 @@
+
+        # need to remove cache file as otherwise prefilled values may be in use
+        call('rm -rf ~/.cache/gnome-boxes/unattended', shell=True)

Good catch!
Comment 42 Zeeshan Ali 2015-06-04 17:01:52 UTC
Review of attachment 304508 [details] [review]:

::: tests/README
@@ -84,3 @@
-* for send key combos test execution
-  * xdotool
-

Actually, you did (in comment#20) so my bad. Having said that "patch hopefully merged now" part isn't true. As I said before, its ok to have dependencies on patches from other bugs but you need to setup the bug deps here.
Comment 43 vladimir benes 2015-06-09 12:23:12 UTC
Created attachment 304852 [details] [review]
tests: Add "Type text and return" step

This step uses xdotool to write specified text into VM drawing area,
and presses return. It works far better for shifted chars than dogtail
and is very handy to execute commands in the VM.
Comment 44 vladimir benes 2015-06-10 11:22:54 UTC
(In reply to Zeeshan Ali (Khattak) from comment #35)
> Review of attachment 304502 [details] [review] [review]:
> 
> All comments in comment#9 addressed but one:
> 
> "* Splitting this into a separate patch reveals an minor issue: this
> function doesn't seem to belong to express-install.py. How about adding it
> to the same place you add other libvirt domain handling functions in other
> bugs/patches? Its ok to have inter-dependencies between patches of different
> bugs as long as you setup deps between bugs."
> 
> You mentioned on IRC that function is related to installation so we can put
> it under 'installation.py' if you like. While you are at it, you might want

as it's now connected just to express-install I would like to keep it there as one can say verifying test user in VM (other express install function) can go to general or utils as well. So please don't require any other file for just single function. We can move it later to libvirt-utils.py during libvirt domain rewrite.

> to rename it to libvirt_domain_get_install_state() since 'vm' and 'domain'
> are synonyms anyway and it'd make code more readable.

I will rename it.
Comment 45 vladimir benes 2015-06-10 11:26:30 UTC
Created attachment 304948 [details] [review]
tests: Add libvirt_domain_get_install_state()

This function can be used for VM state checking from libvirt. It
can say whether system is installing or installed.
Comment 46 vladimir benes 2015-06-10 11:29:02 UTC
Created attachment 304949 [details] [review]
tests: Add "Installation finished check" step

Step that asks libvirt every minute if VM state is installed. Step
fails after user specified amount of time if not.
Comment 47 vladimir benes 2015-06-10 11:31:03 UTC
Created attachment 304950 [details] [review]
tests: Add "Verify user with password" step

Check over ssh (using pexpect) that user is able to log into VM with
password.
Comment 48 vladimir benes 2015-06-10 11:32:48 UTC
Created attachment 304951 [details] [review]
tests: Handle setup page in create_new_vm_via_url

Create new box from URL step is now able to handle express install
screen.
Comment 49 vladimir benes 2015-06-10 11:34:07 UTC
Created attachment 304952 [details] [review]
tests: Increase timeout for media download

Tests are now waiting for media download up to 60 minutes as it's
needed for large installation image downloads like Fedora 20 has.
Comment 50 vladimir benes 2015-06-10 11:47:00 UTC
Created attachment 304953 [details] [review]
tests: Add "Type text and return" step

This step uses xdotool to write specified text into VM drawing area,
and presses return. It works far better for shifted chars than dogtail
and is very handy to execute commands in the VM.
Comment 51 vladimir benes 2015-06-10 11:55:19 UTC
Created attachment 304954 [details] [review]
tests: No implicit return in "Type text" step"

Remove return at the end of "Type text" step.

All original "Type text" steps that needs return are ported to
"Type text and return" step instead.

Xdotool is moved from "Send key combos" test dependency to general
one.
Comment 52 vladimir benes 2015-06-10 12:02:05 UTC
Created attachment 304955 [details] [review]
tests: Add "Express install Fedora 20" test

Add test which downloads Fedora 20 4.3 GB iso and starts express
installation with it. Test user with password is added and checked
via ssh if present after installation.

New hard dependency to pexpect recorded in README file.
Comment 53 vladimir benes 2015-06-10 12:45:19 UTC
Created attachment 304966 [details] [review]
tests: Add "Express install Fedora 20" test

Add test which downloads Fedora 20 4.3 GB iso and starts express
installation with it. Test user with password is added and checked
via ssh if present after installation.
Comment 54 Zeeshan Ali 2015-06-10 16:32:27 UTC
Review of attachment 304948 [details] [review]:

ACK
Comment 55 Zeeshan Ali 2015-06-10 16:41:36 UTC
Review of attachment 304949 [details] [review]:

I'll remove "user" from log to avoid giving the wrong impression that user (as in person using) is providing the timeout.
Comment 56 Zeeshan Ali 2015-06-10 17:01:27 UTC
Review of attachment 304950 [details] [review]:

ack
Comment 57 Zeeshan Ali 2015-06-10 17:02:50 UTC
Review of attachment 304951 [details] [review]:

Could you please put the quoted step name inside ""?
Comment 58 Zeeshan Ali 2015-06-10 17:03:50 UTC
Review of attachment 304952 [details] [review]:

ack
Comment 59 Zeeshan Ali 2015-06-10 17:04:25 UTC
Review of attachment 304953 [details] [review]:

ack
Comment 60 Zeeshan Ali 2015-06-10 17:07:42 UTC
Review of attachment 304954 [details] [review]:

Looks good except for one nitpick: From https://wiki.gnome.org/Git/CommitMessages :

"Use imperative form of verbs rather than past tense when referring to changes introduced by commit in question. For example "Remove property X" rather than "Removed property X"."

You are following this rule in first para but not the other two.
Comment 61 Zeeshan Ali 2015-06-10 17:09:21 UTC
Review of attachment 304966 [details] [review]:

awesome!
Comment 62 Zeeshan Ali 2015-06-10 17:19:27 UTC
All pushed with changes suggested in the review comments. Thanks so much for working on this!

Attachment 304948 [details] pushed as d12afb0 - tests: Add libvirt_domain_get_install_state()
Attachment 304949 [details] pushed as 0f981d8 - tests: Add "Installation finished check" step
Attachment 304950 [details] pushed as ab35c4c - tests: Add "Verify user with password" step
Attachment 304951 [details] pushed as d5e38f4 - tests: Handle setup page in create_new_vm_via_url()
Attachment 304952 [details] pushed as 1ed9d7c - tests: Increase timeout for media download
Attachment 304953 [details] pushed as 237a3de - tests: Add "Type text and return" step
Attachment 304954 [details] pushed as 9e7a498 - tests: No implicit return in "Type text" step"
Attachment 304966 [details] pushed as 5c07fd5 - tests: Add "Express install Fedora 20" test