Feature #8002

Automatic tests for XMPP in Pidgin

Added by intrigeri 2014-10-05 06:02:51 . Updated 2015-03-23 02:01:50 .

Status:
Resolved
Priority:
Elevated
Assignee:
Category:
Test suite
Target version:
Tails_1.3.1
Start date:
2014-10-05
Due date:
% Done:

100%

Feature Branch:
test/8001-8002-pidgin-xmp-otr
Type of work:
Code
Blueprint:

Starter:
Affected tool:
Instant Messaging
Deliverable for:

Description

Subtasks

History

#1 Updated by intrigeri 2014-10-05 06:03:19

#2 Updated by intrigeri 2014-10-05 06:07:32

#3 Updated by intrigeri 2014-10-05 06:07:50

  • Assignee set to anonym
  • Target version changed from Sustainability_M1 to Tails_1.2.2
  • Parent task changed from Feature #6298 to Feature #7820

#4 Updated by intrigeri 2014-10-20 14:06:11

  • blocks #8117 added

#5 Updated by anonym 2014-12-12 16:41:52

  • Target version changed from Tails_1.2.2 to Tails_1.2.3

#6 Updated by BitingBird 2015-01-07 18:11:03

  • Affected tool set to Instant Messaging

#7 Updated by anonym 2015-01-13 14:14:44

  • Target version changed from Tails_1.2.3 to Tails_1.3

#8 Updated by anonym 2015-02-24 20:04:07

  • Target version changed from Tails_1.3 to Tails_1.3.2

#9 Updated by sajolida 2015-02-26 15:11:07

  • Priority changed from Normal to Elevated

I’m concerned to see that this ticket has been postponed for the fourth time. Take into account that this is blocking a funders’ deliverable (#8117) and cannot be postponed anymore this time. So marking this as Elevated.

#10 Updated by Tails 2015-02-27 15:06:17

  • Status changed from Confirmed to In Progress

Applied in changeset commit:f115aa9b477143fb8ff6e1e187b37a4031565168.

#11 Updated by anonym 2015-02-27 15:37:24

  • Assignee changed from anonym to kytv
  • % Done changed from 0 to 50
  • QA Check set to Ready for QA
  • Feature Branch set to test/8000-8001-pidgin-xmp-otr

#12 Updated by kytv 2015-02-27 23:31:44

  • Assignee changed from kytv to anonym
  • % Done changed from 50 to 80
  • QA Check changed from Ready for QA to Dev Needed

Looks good and works well. I saw a couple of minor (not functionality breaking) typos which I pushed to my repo.

After the typos are fixed, I say, “merge”. :)

#13 Updated by kytv 2015-02-27 23:55:42

Perhaps the new dependencies listed on wiki/src/contribute/release_process/test/usage.mdwn should also be added to the check_dependencies steps in run_test_suite?

#14 Updated by anonym 2015-03-01 20:21:59

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

kytv wrote:
> Looks good and works well. I saw a couple of minor (not functionality breaking) typos which I pushed to my repo.
>
> After the typos are fixed, I say, “merge”. :)

Thanks! I’ve merged your branch. Reassigning to intrigeri.

> Perhaps the new dependencies listed on wiki/src/contribute/release_process/test/usage.mdwn should also be added to the check_dependencies steps in run_test_suite?

It’s already very out-of-date. I opened Feature #8988 to determine its future.

#15 Updated by intrigeri 2015-03-02 18:39:32

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

See review on Feature #8001.

#16 Updated by intrigeri 2015-03-03 10:55:30

It would be good to allow the local test suite configuration to specify the “connect server”: many XMPP services, e.g. riseup.net, use a specific hostname for that, which is not the same as the “Domain”. By not allowing to set this parameter, such services can’t be used in local.yml.

#17 Updated by anonym 2015-03-04 20:49:44

  • Assignee changed from anonym to intrigeri
  • QA Check changed from Dev Needed to Info Needed

intrigeri wrote:
> It would be good to allow the local test suite configuration to specify the “connect server”: many XMPP services, e.g. riseup.net, use a specific hostname for that, which is not the same as the “Domain”. By not allowing to set this parameter, such services can’t be used in local.yml.

Fixed in commit cf871ad. Note that Riseup accounts work fine without a Connect Server when Tor is not used, so it’s not needed for the bot.

However, when it comes to Riseup + Tor + the Connect Server .onion, we’ll get a certificate verification prompt. To deal with this particular indeterminism is actually surprisingly hard: we want to wait for either of:

  • a picture showing that we connected fine
  • a picture showing the verification prompt

and do different things depending on which we got. While Sikuli has a waitAll(), it doesn’t have a waitAny(), which is what we’d need here. We could implement a waitAny() ourselves using find(), but it’ll be hard and ugly because of the --retry-find stuff and in particular since we’ve already overriden find(), but we’ll always need the original one without the --retry-find-hook. Urgh.

A simpler approach is perhaps to add another option to the XMPP Pidgin account format (in local.yml) that’s called expect_cert_verification, and if it’s set to true then we make the scenario deal with the prompt (and it’ll fail if there’s none).

What do you think?

#18 Updated by intrigeri 2015-03-04 21:13:12

> What do you think?

I think it’s not worth dealing with certificate verification at all. This topic seems orthogonal to the question at hand, though, assuming that Riseup’s XMPP “connect server”, that is xmpp.riseup.net (https://kingant.net/check_xmpp_dns/?h=riseup.net), has a “valid” certificate. Isn’t that the case?

#19 Updated by intrigeri 2015-03-04 21:16:40

  • % Done changed from 80 to 90
  • QA Check changed from Info Needed to Ready for QA

OK, so I guess that on this ticket, next step is that I should review the new commits and test the “connect server” support, right?

#20 Updated by anonym 2015-03-06 00:41:11

intrigeri wrote:
> > What do you think?
>
> I think it’s not worth dealing with certificate verification at all. This topic seems orthogonal to the question at hand, though, assuming that Riseup’s XMPP “connect server”, that is xmpp.riseup.net (https://kingant.net/check_xmpp_dns/?h=riseup.net), has a “valid” certificate. Isn’t that the case?

Ah, I was confused. I though if you used Tor you had to use the .onion. Great! Nevermind! :)

> OK, so I guess that on this ticket, next step is that I should review the new commits and test the “connect server” support, right?

Yes, now it should be in a better state. I actually had pushed a bug, which now is fixed (commit d47bdae). I suppose that’s the risk when programming with a fever (of the sickness type). :)

#21 Updated by intrigeri 2015-03-06 08:07:59

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

See Feature #8001.

#22 Updated by anonym 2015-03-07 01:20:41

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

#23 Updated by intrigeri 2015-03-07 12:30:13

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

Code review passes except two nitpicking comments:

  • (0..len-1).map { |n| set.sample }.join seems overly complicated, while we could presumably do set.sample(len).join, no? If you agree it makes sense, I can apply this change and test it locally before merging.
  • I would drop the generate_ prefix in generate_random_alnum_string and friends, since I generally prefer naming functions that have no side effect depending on what they return, instead of what they do, but that’s surely a matter of style. Same as above, if you’re fine with that I can do it myself.

#24 Updated by intrigeri 2015-03-07 12:31:55

  • Feature Branch changed from test/8000-8001-pidgin-xmp-otr to test/8001-8002-pidgin-xmp-otr

#25 Updated by intrigeri 2015-03-09 11:18:06

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

intrigeri wrote:
> Code review passes except two nitpicking comments:

Both implemented in branch test/8001-8002-pidgin-xmp-otr-intrigeri, I’ll go ahead, do my testing on that one, and then either will be bold (push, then ask for review and forgiveness) or ask anonym to ack it first.

#26 Updated by anonym 2015-03-09 11:56:44

intrigeri wrote:
> Code review passes except two nitpicking comments:
>
> * (0..len-1).map { |n| set.sample }.join seems overly complicated, while we could presumably do set.sample(len).join, no? If you agree it makes sense, I can apply this change and test it locally before merging.

Note that sample(n) picks a sample of n elements without repetition.

> * I would drop the generate_ prefix in generate_random_alnum_string and friends, since I generally prefer naming functions that have no side effect depending on what they return, instead of what they do, but that’s surely a matter of style. Same as above, if you’re fine with that I can do it myself.

Fixed!

> Both implemented in branch test/8001-8002-pidgin-xmp-otr-intrigeri, I’ll go ahead, do my testing on that one, and then either will be bold (push, then ask for review and forgiveness) or ask anonym to ack it first.

I cherry-picked your good commit ccda39c but we really don’t want commit 5fe8d9c. So imho the branch to test and merge is still test/8001-8002-pidgin-xmp-otr.

#27 Updated by intrigeri 2015-03-09 12:36:47

  • blocks Feature #8966: Automatic tests for IRC CTCP in Pidgin added

#28 Updated by intrigeri 2015-03-09 12:39:14

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

The following test results come from a test suite run from current stable + branches test/7820-pidgin-irc-ctcp and test/8001-8002-pidgin-xmp-otr-intrigeri merged into it.

I see:

Virtual X framebuffer started on display :1
VNC server running on: localhost:5900
/srv/git/features/support/helpers/misc_helpers.rb:115: warning: already initialized constant ALPHA_SET
/srv/git/features/support/helpers/misc_helpers.rb:116: warning: already initialized constant ALNUM_SET

=> apparently misc_helpers.rb is loaded twice, pretty early in the test suite run process. Perhaps these two constants should go into config.rb? Alternatively, given how little they’re used and how cheap they are, they could simply become variables in the functions that use them, at least until we need them somewhere else.

I also see:

    When I join some empty multi-user chat                                 # features/step_definitions/pidgin.rb:117
      FindFailed: can not find PidginChat1UserInRoom.png on the screen.
      Line ?, in File ? (RuntimeError)
      features/pidgin.feature:35:in `When I join some empty multi-user chat'

… and the failure snapshot displays a dialog window titled “Create New Room”, with two buttons: “Accept Defaults” and “Configure Room”. I’ve not configured a chat_room field in local.yml, and my Tails_account is using im.apinc.org. I suspect that the “I join some empty multi-user chat” step doesn’t support creating a MUC, does it? If it’s too complicated to implement, please just document clearly the requirement that the MUC has already been created.

#29 Updated by anonym 2015-03-09 14:02:25

intrigeri wrote:
> The following test results come from a test suite run from current stable + branches test/7820-pidgin-irc-ctcp and test/8001-8002-pidgin-xmp-otr-intrigeri merged into it.
>
> I see:
>
> […]
>
> => apparently misc_helpers.rb is loaded twice, pretty early in the test suite run process.

This is probably caused by require "[...]/misc_helpers.rb" in config.rb. The order the files are loaded by cucumber is something of a mystery to me. Any way, require:ing something multiples times shouldn’t be a problem, but it becomes that when you have constant definitions in the global context, since they never should be redefined.

> Perhaps these two constants should go into config.rb? Alternatively, given how little they’re used and how cheap they are, they could simply become variables in the functions that use them, at least until we need them somewhere else.

I’ll go with the last proposal for now, see commit:8efef71.

> I also see:
>
> […]
>
> … and the failure snapshot displays a dialog window titled “Create New Room”, with two buttons: “Accept Defaults” and “Configure Room”. I’ve not configured a chat_room field in local.yml, and my Tails_account is using im.apinc.org. I suspect that the “I join some empty multi-user chat” step doesn’t support creating a MUC, does it?

Interesting. This didn’t happen with riseup.net.

> If it’s too complicated to implement, please just document clearly the requirement that the MUC has already been created.

I’ll have a look. This type of conditional (e.g. “do $THIS if $IMAGE1 is seen first, do $THAT if $IMAGE2 is seen first”) is kinda tricky, and I suspect it won’t look pretty.

#30 Updated by anonym 2015-03-12 22:58:32

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

anonym wrote:
> intrigeri wrote:
> > I also see:
> >
> > […]
> >
> > … and the failure snapshot displays a dialog window titled “Create New Room”, with two buttons: “Accept Defaults” and “Configure Room”. I’ve not configured a chat_room field in local.yml, and my Tails_account is using im.apinc.org. I suspect that the “I join some empty multi-user chat” step doesn’t support creating a MUC, does it?
>
> Interesting. This didn’t happen with riseup.net.
>
> > If it’s too complicated to implement, please just document clearly the requirement that the MUC has already been created.
>
> I’ll have a look. This type of conditional (e.g. “do $THIS if $IMAGE1 is seen first, do $THAT if $IMAGE2 is seen first”) is kinda tricky, and I suspect it won’t look pretty.

Fixed in commit:872088a (bonus refactoring in commit:22e3463).

#31 Updated by intrigeri 2015-03-14 09:01:05

Code review passes. I’ll try to test it during the week-end.

#32 Updated by intrigeri 2015-03-15 08:57:36

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

I’m following up here on the discussion that started on Feature #8001#note-31, since it is XMPP-specific and has nothing to do with OTR.

With the local.yml you’ve sent me, “my XMPP friend goes online” consistently fails, so I cannot validate this test with these account details (two Riseup ones) either.

It seems that otr-bot.py needs to be run on a system that can resolve SRV DNS queries: if I run it by hand, I see An error occurred while looking up _xmpp-client._tcp.riseup.net. During the test suite run, I see that process trying to connect on port 5222 to riseup.net’s IP, while it should connect to xmpp.riseup.net. Could you please add support for connect_server to the Friend_account, as was done for the Tails_account already? Otherwise, this new environment requirement for running the automated test suite should be documented somewhere (I’ve no idea how I personally will satisfy it, and other developers may be in the same situation, so better try to find a better solution first).

#33 Updated by intrigeri 2015-03-15 09:12:30

  • blocked by deleted (Feature #8966: Automatic tests for IRC CTCP in Pidgin)

#34 Updated by anonym 2015-03-16 15:39:57

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

intrigeri wrote:
> I’m following up here on the discussion that started on Feature #8001#note-31, since it is XMPP-specific and has nothing to do with OTR.
>
> With the local.yml you’ve sent me, “my XMPP friend goes online” consistently fails, so I cannot validate this test with these account details (two Riseup ones) either.
>
> It seems that otr-bot.py needs to be run on a system that can resolve SRV DNS queries: if I run it by hand, I see An error occurred while looking up _xmpp-client._tcp.riseup.net. During the test suite run, I see that process trying to connect on port 5222 to riseup.net’s IP, while it should connect to xmpp.riseup.net. Could you please add support for connect_server to the Friend_account, as was done for the Tails_account already? Otherwise, this new environment requirement for running the automated test suite should be documented somewhere (I’ve no idea how I personally will satisfy it, and other developers may be in the same situation, so better try to find a better solution first).

Ok. Adding that to Jabberbot isn’t very pretty, but I guess we have no choice. Fixed in commit:5852a01.

#35 Updated by intrigeri 2015-03-16 20:48:23

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

Code review passes, and pidgin.feature now passes for me. Yay!

Please merge current stable into this branch, and do what you said on Feature #8966#note-20 to avoid code duplication. Then, back to Feature #8001 and Feature #9062.

#36 Updated by anonym 2015-03-17 08:39:08

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

intrigeri wrote:
> Code review passes, and pidgin.feature now passes for me. Yay!

\o/

> Please merge current stable into this branch, and do what you said on Feature #8966#note-20 to avoid code duplication.

Done.

#37 Updated by intrigeri 2015-03-17 12:04:26

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

Applied in changeset commit:ff9063e75d95029c454b763b4d6298dd802ec022.

#38 Updated by intrigeri 2015-03-17 12:13:04

  • Assignee deleted (intrigeri)
  • QA Check changed from Ready for QA to Pass

#39 Updated by BitingBird 2015-03-22 11:55:30

  • Target version changed from Tails_1.3.2 to Tails_1.3.1

#40 Updated by BitingBird 2015-03-23 02:01:50

  • Status changed from Fix committed to Resolved