Project

General

Profile

Feature #12059

Improve Dogtail's performance

Added by anonym over 2 years ago. Updated over 2 years ago.

Status:
Resolved
Priority:
Normal
Assignee:
-
Category:
Test suite
Target version:
Start date:
12/21/2016
Due date:
% Done:

100%

Feature Branch:
test/12059-dogtail-optimizations
Type of work:
Code
Blueprint:
Starter:
Affected tool:

Description

Currently we run each command in its own script, importing all modules and initiating Dogtail each time, and many commands depend on re-compute previous results. Our .children() method is particularly bad in this respect.

We should instead run all Dogtail commands in a persistent Python session so results can be stored between Dogtail commands.


Related issues

Blocked by Tails - Feature #11887: Make the remote shell's file operations robust Resolved 10/25/2016

Associated revisions

Revision 7c1e999c (diff)
Added by anonym over 2 years ago

Remote shell: extend with persistent Python 2.7 per-user sessions.

  • "persistent": effects (e.g. assignments) survive between separate
    remote shell python commands.
  • "Python 2.7": because that is what Dogtail needs.
  • "per-user": the `dogtail` module must be imported as the user
    running the applications you intend to have Dogtail interact with.

This will allow us to optimize the performance of Dogtail
significantly, as well as reduce the code complexity of the Dogtail
wrapper.

Refs: #12059

Revision 1558f125 (diff)
Added by anonym over 2 years ago

Dogtail: use the remote shell's new Python session feature.

... to significantly improve Dogtail's performance by saving state and
reusing it between Dogtail commands.

This is a massive commit, and it changes the semantics of the creation
of Dogtail objects. Previously they just created the code that then
would be run once an actionable method was called (.wait, .click etc),
but now it works like in Python, that Dogtail will try to find the
graphical element upon object creation.

Will-fix: #12059

Revision 3e0531ab
Added by intrigeri over 2 years ago

Merge remote-tracking branch 'origin/test/12059-dogtail-optimizations' into stable (Fix-committed: #12059).

Revision 403eb3fc (diff)
Added by anonym over 2 years ago

Test suite: adapt tests after change of semantics for Dogtail.

This is vs #12059, which changes the semantics of Dogtail so that
nodes are searched for on Dogtail::* object creation.

Refs: #12059

History

#1 Updated by anonym over 2 years ago

  • Status changed from Confirmed to In Progress
  • % Done changed from 0 to 20
  • QA Check set to Dev Needed
  • Feature Branch set to wip/test/12059-dogtail-optimizations

The WIP branch works for some small ad-hoc tests I've done. I have not tried it in the actual test suite yet.

#2 Updated by anonym over 2 years ago

  • Blocked by Feature #11887: Make the remote shell's file operations robust added

#3 Updated by anonym over 2 years ago

  • % Done changed from 20 to 40
  • Feature Branch changed from wip/test/12059-dogtail-optimizations to test/12059-dogtail-optimizations

Seems to work just fine! Let's see if Jenkins agrees.

#4 Updated by anonym over 2 years ago

  • Assignee changed from anonym to intrigeri
  • % Done changed from 40 to 50
  • QA Check changed from Dev Needed to Ready for QA

Of the two Jenkins runs so far, all failures were unrelated.

#5 Updated by anonym over 2 years ago

Note that compared to its base branch (stable) the runs are only a few minutes faster. That's not impressive. However:

  • we don't use Dogtail too much yet. Once we do, this will start to pay off.
  • at least I have refrained myself from using e.g. .children() because its performance was absolutely horrible, so => more expressiveness.
  • it feels good to have semantics more similar to the real Dogtail (i.e. creating a node object means finding a match) which would be extremely inefficient to implement without object persistence.

#6 Updated by intrigeri over 2 years ago

  • at least I have refrained myself from using e.g. .children() because its performance was absolutely horrible, so => more expressiveness.

If that's something that other people writing tests should be careful about, I suggest documenting it :)

#7 Updated by intrigeri over 2 years ago

intrigeri wrote:

  • at least I have refrained myself from using e.g. .children() because its performance was absolutely horrible, so => more expressiveness.

If that's something that other people writing tests should be careful about, I suggest documenting it :)

Maybe I got it wrong: did you mean that if you hadn't refrained from using .children(), the performance improvements might be more obvious?

#8 Updated by intrigeri over 2 years ago

  • Assignee changed from intrigeri to anonym
  • % Done changed from 50 to 60
  • QA Check changed from Ready for QA to Info Needed

Impressive work! Code review passes at 27370365ad57b5328e3470413f5ba95490c52f8a, modulo:

  • The last 2 test runs on Jenkins show one failing Icedove test, that I don't recall seing fail this way earlier, so I wonder if this might be caused by something related to this branch or to #11887. Could you please have a(nother) look and ensure we have a ticket about this failure, if it's truly unrelated to these 2 branches?
  • So we're generating Python 2.x code from Ruby, sending it to a Python 3 program that will execute it in a Python 2.x subprocess. Wow :) At some point we might need to add some design doc for this kind of things, so that one (e.g. myself) can easily refresh the big picture in their mind without having to go through the entire code path. I wonder if it's viable on the long run to continue adding complex bits like this while postponing the design doc writing. Note that we already have a "Remote shell" section in the test suite design doc; adding pointers to the client side (test suite code that interacts with the remote shell) would already be a time saver. Not a blocker, your call!
  • I find it rather counter-intuitive that init_script is an array of strings, but I can live with it.
  • It's great that the new semantics look closer to what Dogtail users would expect IMO.
  • Reading the message for 01f06c13879f905653adb4f0aef6b677b24c2383 makes me wonder if something should be done upstream about it.

#9 Updated by anonym over 2 years ago

  • Assignee changed from anonym to intrigeri
  • QA Check changed from Info Needed to Ready for QA

intrigeri wrote:

Impressive work! Code review passes at 27370365ad57b5328e3470413f5ba95490c52f8a, modulo:

  • The last 2 test runs on Jenkins show one failing Icedove test, that I don't recall seing fail this way earlier, so I wonder if this might be caused by something related to this branch or to #11887. Could you please have a(nother) look and ensure we have a ticket about this failure, if it's truly unrelated to these 2 branches?

If you look at the videos, you can see that Icedove fails to start. Filed as #12183 (although I think all Icedove crashes are related, so it could as well be #12046 with a generalized title).

  • So we're generating Python 2.x code from Ruby, sending it to a Python 3 program that will execute it in a Python 2.x subprocess. Wow :)

Actually, dogtail 0.9.9 supports Python 3, so we could move to Ruby + Python 3 only. Yay! Opened #12185 (targeting 3.0 for now since that was my original plan).

At some point we might need to add some design doc for this kind of things, so that one (e.g. myself) can easily refresh the big picture in their mind without having to go through the entire code path. I wonder if it's viable on the long run to continue adding complex bits like this while postponing the design doc writing. Note that we already have a "Remote shell" section in the test suite design doc; adding pointers to the client side (test suite code that interacts with the remote shell) would already be a time saver. Not a blocker, your call!

Well, I won't do it now, since it will change soon.

  • I find it rather counter-intuitive that init_script is an array of strings, but I can live with it.

Ack. I find multiline strings awkward to work with, though, but I'm open for a better variable name. This was not introduced by this branch, though, so blocking on it would be weird. :)

  • It's great that the new semantics look closer to what Dogtail users would expect IMO.

Yeah!

If you merge, I'll file a ticket about investigating this, ok?

#10 Updated by intrigeri over 2 years ago

  • Assignee changed from intrigeri to anonym
  • QA Check changed from Ready for QA to Info Needed

If you look at the videos, you can see that Icedove fails to start. Filed as #12183 (although I think all Icedove crashes are related, so it could as well be #12046 with a generalized title).

Well, yeah, OK, but it happens on every test suite run on this branch, and I've just looked at the last few runs on stable and devel, that don't expose the bug. So this looks very much like a regression introduced by this branch, no? I'll happily dismiss this if I'm shown occurrences of this problem on branches that haven't the changes brought by this one.

  • I find it rather counter-intuitive that init_script is an array of strings, but I can live with it.

Ack. I find multiline strings awkward to work with, though, but I'm open for a better variable name. This was not introduced by this branch, though, so blocking on it would be weird. :)

This branch does rename init_lines to init_script, hence my comment. Reverting to init_lines would address my concern.

If you merge, I'll file a ticket about investigating this, ok?

OK.

Also, see my question on #12059#note-7.

#11 Updated by anonym over 2 years ago

  • Assignee changed from anonym to intrigeri

intrigeri wrote:

If you look at the videos, you can see that Icedove fails to start. Filed as #12183 (although I think all Icedove crashes are related, so it could as well be #12046 with a generalized title).

Well, yeah, OK, but it happens on every test suite run on this branch, and I've just looked at the last few runs on stable and devel, that don't expose the bug. So this looks very much like a regression introduced by this branch, no? I'll happily dismiss this if I'm shown occurrences of this problem on branches that haven't the changes brought by this one.

Ok, it turns out we just wanted to wait longer for Icedove to start (=> rejected #12183). I suspect that that default is just slightly too little, and the reason we saw only the first Icedove scenario fail might be due to caching making it slightly faster for subsequent runs.

The same scenario still has an issue later on when trying to click the "Display Expert Settings and Menus" button which I tried to fix with cf44cb3, but that didn't work out. I've pushed something else so let's see how it goes for the next few Jenkins jobs (after #27).

As for the rest of the failures I see for this branch, I also see them in the stable and devel branches.

  • I find it rather counter-intuitive that init_script is an array of strings, but I can live with it.

Ack. I find multiline strings awkward to work with, though, but I'm open for a better variable name. This was not introduced by this branch, though, so blocking on it would be weird. :)

This branch does rename init_lines to init_script, hence my comment. Reverting to init_lines would address my concern.

Fixed!

If you merge, I'll file a ticket about investigating this, ok?

OK.

#12191

Also, see my question on #12059#note-7.

This branch will make our .children() efficient, so there's nothing to document. And sure, if we had used it more before this branch, with this branch we'd see a bigger performance boost.

#12 Updated by intrigeri over 2 years ago

  • Status changed from In Progress to Fix committed
  • % Done changed from 60 to 100

#13 Updated by intrigeri over 2 years ago

  • Assignee deleted (intrigeri)
  • QA Check changed from Info Needed to Pass

Merged! Note that there are conflicts when mergig into feature/stretch so I didn't do it.

#14 Updated by anonym over 2 years ago

intrigeri wrote:

Merged!

Woo! Thanks!

Note that there are conflicts when mergig into feature/stretch so I didn't do it.

I merged, resolved the conflicts, and adapted various usage of Dogtail introduced in feature/stretch to the new Dogtail semantics (mostly: .wait doesn't exist, and is instead implicit when creating the Dogtail object).

#15 Updated by anonym over 2 years ago

  • Status changed from Fix committed to Resolved

Also available in: Atom PDF