GNOME Bugzilla – Bug 751042
various tests speed optimizations and slower machine stabilizations
Last modified: 2016-09-20 08:15:55 UTC
As creating snapshots is kid of slower these days (in Fedora 22) I am slightly increasing timeout of creation. This is not gnome boxes issue but more likely qemu's and of course hdd speed takes some part. On the other hand I've removed some 5 seconds long waits. These seems to be unnecessary now.
Created attachment 305386 [details] [review] tests: Increase snapshot creation timeout Timeout for snaphot creation changes from 5 seconds to 20. Snapshot readiness is checked every 0.25s instead of 1 second.
Created attachment 305387 [details] [review] tests: Remove 5s delays in snapshot test Remove 5 second delays from "Create snapshots and revert" test.
Review of attachment 305386 [details] [review]: Justification for changes in the log please. ::: tests/steps/snapshot.py @@ +21,3 @@ while len(context.app.findChildren(lambda x: x.roleName == 'toggle button' and x.showing \ and x.sensitive and x.name == 'Menu')) == 0: + sleep(0.25) Why are we changing this?
Review of attachment 305387 [details] [review]: Justification missing in the log.
(In reply to Zeeshan Ali (Khattak) from comment #3) > Review of attachment 305386 [details] [review] [review]: > > Justification for changes in the log please. > > ::: tests/steps/snapshot.py > @@ +21,3 @@ > while len(context.app.findChildren(lambda x: x.roleName == 'toggle > button' and x.showing \ > > and x.sensitive and x.name == 'Menu')) == 0: > + sleep(0.25) > > Why are we changing this? test can be quite quicker then if these snapshots are slower this can save some small amount of time
(In reply to Zeeshan Ali (Khattak) from comment #4) > Review of attachment 305387 [details] [review] [review]: > > Justification missing in the log. justification? You mean something like Remove 5 second delays from "Create snapshots and revert" test as they're just slowing the test?
(In reply to vladimir benes from comment #5) > (In reply to Zeeshan Ali (Khattak) from comment #3) > > Review of attachment 305386 [details] [review] [review] [review]: > > > > Justification for changes in the log please. > > > > ::: tests/steps/snapshot.py > > @@ +21,3 @@ > > while len(context.app.findChildren(lambda x: x.roleName == 'toggle > > button' and x.showing \ > > > > and x.sensitive and x.name == 'Menu')) == 0: > > + sleep(0.25) > > > > Why are we changing this? > > test can be quite quicker then if these snapshots are slower this can save > some small amount of time OK, since it has different rationale than main change, separate patch with its own justification (e.g Now that we wait much longer for snapshot creation, better save as much time as possible between checks for snapshot creation).
(In reply to vladimir benes from comment #6) > (In reply to Zeeshan Ali (Khattak) from comment #4) > > Review of attachment 305387 [details] [review] [review] [review]: > > > > Justification missing in the log. > > justification? You mean something like > Remove 5 second delays from "Create snapshots and revert" test as they're > just slowing the test? More like why these are not needed now but were before?
(In reply to Zeeshan Ali (Khattak) from comment #8) > (In reply to vladimir benes from comment #6) > > (In reply to Zeeshan Ali (Khattak) from comment #4) > > > Review of attachment 305387 [details] [review] [review] [review] [review]: > > > > > > Justification missing in the log. > > > > justification? You mean something like > > Remove 5 second delays from "Create snapshots and revert" test as they're > > just slowing the test? > > More like why these are not needed now but were before? "Turns out that these delays were not needed after all" would suffice too.
Created attachment 305589 [details] [review] tests: Remove 5s delays in snapshot test Turns out that these delays were not needed after all
Created attachment 305590 [details] [review] tests: Increase snapshot creation timeout Timeout for snaphot creation changes from 5 seconds to 20.
Created attachment 305591 [details] [review] tests: Check snapshots more frequently Now that we wait much longer for snapshot creation, better save as much time as possible between checks for snapshot creation
Created attachment 305592 [details] [review] tests: Wait for proper shutdown Shutdown of Boxes takes some time in case there are more machines running so we need to wait up to 50 seconds for it.
Created attachment 305597 [details] [review] tests: Speed up machine creation Press 3 times return with 1 second delay instead of sleeping for 3 seconds before pressing return. This may save up to two seconds in every single machine installation.
Created attachment 305598 [details] [review] tests: Ping checks speedup Various ping options makes ping checks far quicker. Successful ping is 1 second quicker and unsuccessful ping check is done in 1 second instead of 10!.
Review of attachment 305589 [details] [review]: Good but some nitpicks: * 'in' -> 'from' * dot missing at the end of sentence.
Review of attachment 305590 [details] [review]: Seems you accidently dropped the explanation/rationale during rebase.
Review of attachment 305591 [details] [review]: Dot missing at end of sentence.
Review of attachment 305592 [details] [review]: Shortlog's incorrect. How about "tests: Wait 50s for shutdown". ::: tests/steps/general.py @@ +129,3 @@ + counter += 1 + if counter == 100: + raise Exception("Cannot turn off Boxes in 50 seconds") More appropriate would be: Failed to turn off..
Review of attachment 305597 [details] [review]: Good catch * "Faster booting of machine" is probably more appropriate shortlog. i-e summarize change rather than benefit in shortlog. * "Press 3 times return" -> "Press "Return" key 3 times". Similarly "pressing return" -> "pressing "Return". * "may" makes it sound like its a guess. I'm sure you tested it to see the benefits at work so I'd be sounding a bit more certain. :) Perhaps "may save" -> "usually saves"?
Review of attachment 305598 [details] [review]: * " tests: Ping checks speedup" -> "tests: Faster ping checks" * I'm as excited about this change as you are, but you eithe want "!" or "." at the end. :) ::: tests/steps/utils.py @@ +39,3 @@ sleep(1) + redundant change.
Created attachment 305666 [details] [review] tests: Harden "Resume livecd box" test Add 4 seconds before pinging after resume a live cd box. This makes test more stable on slower machines.
Created attachment 305667 [details] [review] tests: Harden "System broker machine deleted" test Wait two seconds before checking if machine still exist in Boxes after deleting it via virsh command.
Created attachment 305668 [details] [review] tests: Increase snapshot creation timeout Timeout for snaphot creation changes from 5 seconds to 20 as it is too low for latest Qemu.
Created attachment 305669 [details] [review] tests: Check snapshots more frequently Now that we wait much longer for snapshot creation, better save as much time as possible between checks for snapshot creation.
Created attachment 305671 [details] [review] tests: Wait for proper shutdown Shutdown of Boxes takes some time in case there are more machines running so we need to wait up to 50 seconds for it.
Created attachment 305672 [details] [review] tests: Faster booting of machine Press "Return" key 3 times instead of sleeping for 3 seconds before pressing "Return" key for the first time. This usually saves up to 2 seconds in every single machine installation.
Created attachment 305674 [details] [review] tests: Faster ping checks Various ping options makes ping checks far quicker. Successful ping is 1 second quicker and unsuccessful ping check is done in 1 second instead of 10!
Review of attachment 305666 [details] [review]: * Maybe "tests: Wait longer for livecd boot" would be better summary of actual change? * "Add 4 seconds before pinging after resume a live cd box" -> "Wait 4 seconds rather than just 2, after resuming a live CD box before pinging it". * stable -> reliable. I'll fix the commit log before pushing if commit log's the only thing needing improvement (in all patches)..
Review of attachment 305667 [details] [review]: Similar/same comments are for previous patch.
Review of attachment 305668 [details] [review]: ack
Review of attachment 305669 [details] [review]: ack
Review of attachment 305671 [details] [review]: The part about shortlog got ignored from last review (comment#20).
Review of attachment 305672 [details] [review]: ack
Review of attachment 305674 [details] [review]: ack
All pushed, some with changes to commit log.