Feature #12059

Improve Dogtail's performance

Added by anonym 2016-12-21 15:58:00 . Updated 2017-03-09 13:59:53 .

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

100%

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

Starter:
Affected tool:
Deliverable for:

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.

Subtasks

Related issues

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

History

#1 Updated by anonym 2016-12-21 16:42:15

  • 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 2017-01-25 14:04:43

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

#3 Updated by anonym 2017-01-25 15:55:42

  • % 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 2017-01-26 07:34:21

  • 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 2017-01-26 07:45:16

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 2017-01-26 08:32:33

> * 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 2017-01-27 13:49:42

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 2017-01-27 14:15:22

  • 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 commit: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 Feature #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 commit:01f06c13879f905653adb4f0aef6b677b24c2383 makes me wonder if something should be done upstream about it.

#9 Updated by anonym 2017-01-28 10:42:02

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

intrigeri wrote:
> Impressive work! Code review passes at commit: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 Feature #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 Bug #12183 (although I think all Icedove crashes are related, so it could as well be Bug #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 Feature #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!

> * Reading the message for commit:01f06c13879f905653adb4f0aef6b677b24c2383 makes me wonder if something should be done upstream about it.

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

#10 Updated by intrigeri 2017-01-29 11:28:05

  • 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 Bug #12183 (although I think all Icedove crashes are related, so it could as well be Bug #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.

>> * Reading the message for commit:01f06c13879f905653adb4f0aef6b677b24c2383 makes me wonder if something should be done upstream about it.

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

OK.

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

#11 Updated by anonym 2017-02-16 15:09:49

  • Assignee changed from anonym to intrigeri

intrigeri wrote:
> > If you look at the videos, you can see that Icedove fails to start. Filed as Bug #12183 (although I think all Icedove crashes are related, so it could as well be Bug #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 Bug #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 commit: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!

> >> * Reading the message for commit:01f06c13879f905653adb4f0aef6b677b24c2383 makes me wonder if something should be done upstream about it.
>
> > If you merge, I’ll file a ticket about investigating this, ok?
>
> OK.

Bug #12191

> Also, see my question on Feature #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 2017-03-02 08:07:13

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

Applied in changeset commit:3e0531aba14595b57308fd08c0905f82292ff6f1.

#13 Updated by intrigeri 2017-03-02 08:08:13

  • 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 2017-03-02 09:05:41

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 2017-03-09 13:59:53

  • Status changed from Fix committed to Resolved