Tails

#1 Updated by intrigeri 2013-09-26 06:10:44

• Target version set to Sustainability_M1

#2 Updated by intrigeri 2014-08-08 11:43:02

• related to Feature #7760: Automatically test the effects of the I2P boot parameter added

#3 Updated by intrigeri 2014-08-26 23:20:31

• Subject changed from Write tests for i2p to Write tests for I2P

#4 Updated by BitingBird 2015-01-07 18:12:42

• Affected tool set to I2P

#5 Updated by anonym 2015-01-09 14:11:23

• Assignee set to kytv
• Target version changed from Sustainability_M1 to Tails_1.8

#6 Updated by anonym 2015-01-10 17:19:16

• blocks #8668 added

#7 Updated by anonym 2015-01-10 17:55:16

• Target version changed from Tails_1.8 to Tails_1.4.1

#8 Updated by kytv 2015-01-18 19:13:41

• Status changed from Confirmed to In Progress

#9 Updated by kytv 2015-06-28 13:03:21

• Target version changed from Tails_1.4.1 to Tails_1.5

#10 Updated by intrigeri 2015-08-02 10:54:16

• Due date set to 2015-07-15

#12 Updated by intrigeri 2015-08-02 12:35:45

• Priority changed from Normal to High

#13 Updated by kytv 2015-08-07 15:57:01

• Target version changed from Tails_1.5 to Tails_2.6

#14 Updated by kytv 2015-08-07 15:57:46

• Target version changed from Tails_2.6 to Tails_1.6

#15 Updated by kytv 2015-09-10 13:31:04

• blocked by Bug #10116: "Watching a WebM video" test is fragile added

#16 Updated by kytv 2015-09-10 13:31:22

• % Done changed from 0 to 40

#17 Updated by kytv 2015-09-12 06:52:45

• blocked by Bug #10184: Disable Firefox's suffix and prefix adding in I2P Browser added

#18 Updated by kytv 2015-09-12 06:57:53

• blocked by Bug #10185: Fix tails-i2p's "i2p_has_bootstrapped" function in newer java/newer I2P versions added

#19 Updated by kytv 2015-09-12 17:49:11

• blocked by Bug #9633: waitAny is failing to find images that are on the screen added

#20 Updated by kytv 2015-09-14 08:54:44

• Feature Branch set to kytv:test/6306-tests-for-I2P

This is still a work in progress but I will not rebase anything that I’ve pushed (unless asked to).

#23 Updated by kytv 2015-09-20 15:34:53

• Target version changed from Tails_1.6 to Tails_1.7

#24 Updated by kytv 2015-09-22 05:08:48

• Feature Branch deleted (kytv:test/6306-tests-for-I2P)

#28 Updated by intrigeri 2015-09-22 09:40:09

• blocks deleted (Bug #10116: "Watching a WebM video" test is fragile)

#31 Updated by kytv 2015-10-26 03:04:15

• Assignee changed from kytv to anonym
• QA Check set to Ready for QA
• Feature Branch set to kytv/test/6306-tests-for-I2P

#33 Updated by anonym 2015-10-30 04:59:39

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

I ran only i2p.feature in a loop over the night, until the first failure. I had six successful runs and in the seventh run there was a failure (more about that later). Among the six successful runs the average run time was 26 minutes (and I ran with --keep-snapshots after an initial successful run not included in these stats, so it doesn’t include snapshot creation). That’s quite a lot. I wonder if we can improve the situation. From the top of my head I only have these not very fruitful optimizations; in the following scenarios we could try to restore from snapshots without networking, since it’s not needed and just introduces some unnecessary waiting:

• Scenario: I2P is disabled by default (could restore with ‘I have started Tails from DVD without network and logged in’)

• Scenario: I2P is enabled when the "i2p" boot parameter is added (could restore with ‘I have started Tails from DVD with I2P enabled and logged in’)

Meh. I can only assume that a lot of time is spent on I2P to re-bootstraping (close to the 120 second max in the ‘the I2P router console is ready’ step in each scenario) and that this is because of the issue I raised in Bug #10185, and it will be quite a bit faster once we have the proper fix. A thought: in the with-network-and-i2p snapshot creation we stop at ‘I2P's reseeding completed’, but perhaps we should also throw in ‘I2P successfully built a tunnel’? Or is waiting for the reseed to finish the only thing needed for preparing I2P for a swift restart? Something for later, I guess, sin the issue in Bug #10185 will prevent any speedup from this.

Any way, I think I should merge this despite my concerns about run time, but I think we should file a ticket for benchmarking the I2P tests after the bootstrapping optimization is done (i.e. it should be blocked by the ticket I told you to create in Bug #10185).

Here’s my code review, based on git diff 53c019ced13b57a10d74e4453ec019e140179dac...76c69ee244dea988697816583a039a10a2c04f52. First of all, well done! I can see that a lot of work and though has gone in to this, and everything indeed works really nicely. Only nitpicking remains (and a robustness issue that probably isn’t introduced by this branch):

--- a/features/step_definitions/common_steps.rb
+++ b/features/step_definitions/common_steps.rb
[...]
+        $vm.execute('/usr/local/sbin/tails-i2p stop')
+        $vm.execute('killall tails-i2p')

This looks strange. The call to tails-i2p stop blocks until it exits, so why do we have to kill any remaining instances? Is it because we could have multiple running ones (e.g. since we do the tails-i2p start via spawn)? If so I would like a comment that convinces me that this makes sense. Also, why not $vm.execute_successfully()?

+      $vm.spawn("restart-vidalia")

This branch re-introduces this, which we removed in another branch, since the earlier spawning of restart-tor will make sure Vidalia is running.

-Then /^the I2P Browser desktop file is (|not )present$/ do |mode|
+Then /^the I2P Browser desktop file is (not )?present$/ do |notpresent|
   file = '/usr/share/applications/i2p-browser.desktop'
-  if mode == ''
-    assert($vm.execute("test -e #{file}").success?)
-  elsif mode == 'not '
+  if notpresent
     assert($vm.execute("! test -e #{file}").success?)
   else
-    raise "Unsupported mode passed: '#{mode}'"
+    assert($vm.execute("test -e #{file}").success?)
   end
 end

Instead of an if with almost identical asserts, you could reduce the whole step’s body to this (which imho is clearer):

assert_equal(notpresent.nil?, $vm.execute("test -e #{file}").success?)

There are at least one other place where the same thing can be done.

 def focus_pidgin_irc_conversation_window(account)
-  account = account.sub(/^irc\./, '')
-  $vm.focus_window(".*#{Regexp.escape(account)}$")
+  if account == 'I2P'
+    # After connecting to Irc2P messages are sent from services. Most of the
+    # time the services will send their messages right away. If there's lag we
+    # may in fact join the channel _before_ the message is received. We'll look
+    # for a message from InfoServ first then default to looking for '#i2p'
+    try_for(20) do
+      begin
+        $vm.focus_window('InfoServ')
+      rescue ExecutionFailedInVM
+        $vm.focus_window('#i2p')
+      end
+    end
+  else
+    account = account.sub(/^irc\./, '')
+    try_for(20) do
+      $vm.focus_window(".*#{Regexp.escape(account)}$")
+    end
+  end
 end

Remember this code change and follow me through the only failure I’ve seen while running this feature:

  Scenario: Connecting to the #i2p IRC channel with the pre-configured account                      # features/i2p.feature:54                                     
[...]
    And I can join the "#i2p" channel on "I2P"                                                      # features/step_definitions/pidgin.rb:361                     
      try_for() timeout expired
      Last ignored exception was: ExecutionFailedInVM: Command failed: xdotool search --name '#i2p' windowactivate --sync                                         
      error code: 1
      stderr: .
      <false> is not true. (Timeout::Error)
      ./features/support/helpers/misc_helpers.rb:83:in `rescue in try_for'
      ./features/support/helpers/misc_helpers.rb:33:in `try_for'
      ./features/step_definitions/pidgin.rb:36:in `focus_pidgin_irc_conversation_window'                                                                          
      ./features/step_definitions/pidgin.rb:364:in `/^I can join the "([^"]+)" channel on "([^"]+)"$/'                                                            
      features/i2p.feature:63:in `And I can join the "#i2p" channel on "I2P"'

In the video I can see Pidgin turning the status to fully Available, and “#i2p” appears in the buddy list. I can see some sort of click on the “#i2p” entry, but no conversation window is opened, so the double-click failed.

The earlier ‘Pidgin successfully connects ...’ step has gotten quite complex, and I’m not sure what possible window focus states (including the buggy ones, that you’ve discovered recently) it could end up in. It feels like it should have the buddy list focused, so adding $vm.focus_window('Buddy List') to the beginning of the failing ‘I can join the ...’ step is probably not enough. I guess we need to add some retrying logic for the channel entry double-click. However, then I’m not sure the modification you’ve done focus_pidgin_irc_conversation_window will work out. I think you’ll have to remove the try_for:s and simply make it test each case once.

--- a/features/step_definitions/unsafe_browser.rb
+++ b/features/step_definitions/unsafe_browser.rb

All the steps you’ve modified should move somewhere else now. However, common_steps.rb is getting seriously huge, so I suggest we create a browser.rb that all general browser related stuff is moved into, including everything in common_steps.rb, but not the steps that are still browser specific in e.g. torified_browser.rb, unsafe_browser.rb and i2p.rb.

That’s it! Again, good job!

#34 Updated by kytv 2015-10-31 11:22:15

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

anonym wrote:

> In the video I can see Pidgin turning the status to fully Available, and “#i2p” appears in the buddy list. I can see some sort of click on the “#i2p” entry, but no conversation window is opened, so the double-click failed.
>
> The earlier ‘Pidgin successfully connects ...’ step has gotten quite complex, and I’m not sure what possible window focus states (including the buggy ones, that you’ve discovered recently) it could end up in. It feels like it should have the buddy list focused, so adding $vm.focus_window('Buddy List') to the beginning of the failing ‘I can join the ...’ step is probably not enough. I guess we need to add some retrying logic for the channel entry double-click. However, then I’m not sure the modification you’ve done focus_pidgin_irc_conversation_window will work out. I think you’ll have to remove the try_for:s and simply make it test each case once.

All of the suggested changes have been made, except for this which I’m still testing. I also merged the branch from Bug #10185 as suggested.

#35 Updated by kytv 2015-10-31 11:40:54

• related to Bug #10461: Determine when I2P is ready for use added

#36 Updated by anonym 2015-10-31 13:32:26

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

Here I’ll also review the new changes in bugfix/10185-fix_i2p_start_script_and_bootstrap_checking_function which btw wasn’t pushed to your remote.

commit eb88da592b5d5476e65ff6a2203bb25a07b42547
Merge: 6a0170b 53d10a2
Author: kytv <killyourtv@i2pmail.org>
Date:   Wed Oct 28 13:32:26 2015 +0000

    Merge branch 'devel' into bugfix/10185-fix_i2p_start_script_and_bootstrap_checking_function

Normally this is a problem, post-freeze, since new features (or other things not targeting the release testing is aimed for) could have been merged into devel. Luckily only config/base_branch has to be restored this time. :)

 wait_until_i2p_builds_a_tunnel() {
+    # static sleep to work around upstream bug.
+    sleep 240
     wait_until ${I2P_TUNNEL_BUILD_TIMEOUT} i2p_built_a_tunnel
 }

Wouldn’t it make more sense to do it the other way around, i.e. first wait_until ... i2p_built_a_tunnel (which succeeds too early) and then sleep (to work around that too early success)?

-    And the I2P Browser sudo rules are enabled
+    And the I2P Browser sudo rules are present

Good, I missed this one! But I’d prefer that it would be done in a separate commit. :) Also, as for the commit message of this commit, commit:6325719, I wouldn’t call this “Optimize steps” but a “Code simplification”. Just something for the future.

commit 2ebb903145f34f0a13bc0315cbe8e4a5efcb66aa
Author: kytv <killyourtv@i2pmail.org>
Date:   Fri Oct 30 22:56:46 2015 +0000

    Add explanatory comment

First, with that commit message I wouldn’t expect switching from one method (execute) to another (execute_successfully) => separate commit next time. Also, why do we not use execute_successfully for tails-i2p stop?

Next,

+        # we "killall tails-i2p" to prevent multiple
+        # copies of the script from running

Wow! How the frak is this happening? This sounds super-error prone. :/ Or do you have some argument for this being safe?

#39 Updated by kytv 2015-10-31 13:56:15

• File i2p-err.png added

anonym wrote:

> The earlier ‘Pidgin successfully connects ...’ step has gotten quite complex, and I’m not sure what possible window focus states (including the buggy ones, that you’ve discovered recently) it could end up in. It feels like it should have the buddy list focused, so adding $vm.focus_window('Buddy List') to the beginning of the failing ‘I can join the ...’ step is probably not enough. I guess we need to add some retrying logic for the channel entry double-click. However, then I’m not sure the modification you’ve done focus_pidgin_irc_conversation_window will work out. I think you’ll have to remove the try_for:s and simply make it test each case once.

With trying once I got the following error because of lag from services.

    Then Pidgin successfully connects to the "I2P" account                                          # features/step_definitions/pidgin.rb:315
    And I can join the "#i2p" channel on "I2P"                                                      # features/step_definitions/pidgin.rb:357
      FindFailed: can not find PidginI2PChannelWelcome.png on the screen.
      Line ?, in File ? (RuntimeError)
      features/i2p.feature:63:in `And I can join the "#i2p" channel on "I2P"'

#40 Updated by kytv 2015-11-01 07:30:27

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

Changes made/pushed.

#41 Updated by anonym 2015-11-02 14:35:08

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

Applied in changeset commit:bdc1cc730715eca28928646025b0021fe191f5d5.

#42 Updated by anonym 2015-11-02 16:00:39

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

#43 Updated by anonym 2015-11-03 11:33:26

• Status changed from Fix committed to Resolved