Project

General

Profile

Feature #6306

Feature #6298: Write more automated tests

Write tests for I2P

Added by bertagaz almost 6 years ago. Updated almost 4 years ago.

Status:
Resolved
Priority:
High
Assignee:
-
Category:
Test suite
Target version:
Start date:
09/26/2013
Due date:
07/15/2015
% Done:

100%

Feature Branch:
kytv/test/6306-tests-for-I2P
Type of work:
Code
Blueprint:
Starter:
No
Affected tool:
I2P

Description

If we keep i2p in Tails, there should be tests for it in our test suite.

i2p-err.png View (89.3 KB) kytv, 10/31/2015 01:53 PM


Related issues

Related to Tails - Feature #7760: Automatically test the effects of the I2P boot parameter Resolved 08/08/2014
Related to Tails - Bug #10461: Determine when I2P is ready for use Confirmed 10/31/2015
Blocked by Tails - Feature #5889: have a maintainer for I2P or remove it Resolved 07/19/2013
Blocked by Tails - Bug #10184: Disable Firefox's suffix and prefix adding in I2P Browser Resolved 09/12/2015
Blocked by Tails - Bug #10185: Fix tails-i2p's "i2p_has_bootstrapped" function in newer java/newer I2P versions Resolved 09/12/2015
Blocked by Tails - Bug #9633: waitAny is failing to find images that are on the screen Resolved 06/21/2015

Associated revisions

Revision a9cabac7 (diff)
Added by kytv about 4 years ago

Remove the manual I2P tests — these have been automated ☺

Will-fix: #6306

Revision bdc1cc73
Added by anonym almost 4 years ago

Merge remote-tracking branch 'kytv/test/6306-tests-for-I2P' into testing

Conflicts:
features/support/helpers/misc_helpers.rb

Fix-committed: #6306, #10185

History

#1 Updated by intrigeri almost 6 years ago

  • Target version set to Sustainability_M1

#2 Updated by intrigeri about 5 years ago

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

#3 Updated by intrigeri about 5 years ago

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

#4 Updated by BitingBird over 4 years ago

  • Affected tool set to I2P

#5 Updated by anonym over 4 years ago

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

#7 Updated by anonym over 4 years ago

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

#8 Updated by kytv over 4 years ago

  • Status changed from Confirmed to In Progress

#9 Updated by kytv about 4 years ago

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

#10 Updated by intrigeri about 4 years ago

  • Due date set to 07/15/2015

#12 Updated by intrigeri about 4 years ago

  • Priority changed from Normal to High

#13 Updated by kytv about 4 years ago

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

#14 Updated by kytv about 4 years ago

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

#15 Updated by kytv about 4 years ago

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

#16 Updated by kytv about 4 years ago

  • % Done changed from 0 to 40

#17 Updated by kytv about 4 years ago

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

#18 Updated by kytv about 4 years ago

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

#19 Updated by kytv about 4 years ago

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

#20 Updated by kytv about 4 years ago

  • 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).

#21 Updated by intrigeri about 4 years ago

Note that #9633 has been resolved.

#22 Updated by intrigeri about 4 years ago

I don't get why this is blocked by #10116. But if that's obvious to you, nevermind :)

#23 Updated by kytv almost 4 years ago

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

#24 Updated by kytv almost 4 years ago

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

#25 Updated by kytv almost 4 years ago

intrigeri wrote:

I don't get why this is blocked by #10116. But if that's obvious to you, nevermind :)

Quite simply, we can't test I2P things over the network until I2P is ready and this check is supposed to tell users when I2P can be used.

Since i2p_has_bootstrapped is really for the users—and the tests are mostly to ensure that things work for Tails' users—I'm depending on #10116 instead of adding the functionality to the test suite.

#26 Updated by intrigeri almost 4 years ago

Quite simply, we can't test I2P things over the network until I2P is ready and this
check is supposed to tell users when I2P can be used.

Since i2p_has_bootstrapped is really for the users—and the tests are mostly to
ensure that things work for Tails' users—I'm depending on #10116 instead of adding
the functionality to the test suite.

This looks more like #10185 than #10116 to me, but whatever.

#27 Updated by kytv almost 4 years ago

Oh crap, sorry. Dunno how I got that wrong. (well, yes, I kinda do. I was going "from memory" and misremembered that that was that ticket.)

That was because of the retry_tor function. I want to use the logic of it (but without the forcing a new tor circuit). Since it's been merged it's a redmine thing.

It's fine if we want to unlink it within Redmine, of course.

#28 Updated by intrigeri almost 4 years ago

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

#29 Updated by intrigeri almost 4 years ago

It's fine if we want to unlink it within Redmine, of course.

Done, because being able to rely on such info helps when writing reports etc. :)

#30 Updated by sajolida almost 4 years ago

This was due on July 15, please make sure we can at least deliver in time for the next milestone on October 15.

#31 Updated by kytv almost 4 years ago

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

#32 Updated by anonym almost 4 years ago

While this ticket is blocked by #10185, it's not clear that this ticket's branch depends on that ticket's branch. I'd say that the correct thing to do here is to merge kytv/bugfix/10185-fix_i2p_start_script_and_bootstrap_checking_function into kytv/test/6306-tests-for-I2P.

#33 Updated by anonym almost 4 years ago

  • 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 #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 #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 #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 almost 4 years ago

  • 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 #10185 as suggested.

#35 Updated by kytv almost 4 years ago

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

#36 Updated by anonym almost 4 years ago

  • 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, 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?

#37 Updated by anonym almost 4 years ago

anonym wrote:

Next,

[...]

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

Ah, could this be related to NetworkManager having stray hooks from previous connections? Let's say we have NM hooks X, Y, Z on "interface up". We connect to some network and hook X runs and exits, and Y is running, and it runs for a long time for some reason. We then disconnect even before Y exits. Then we connect again. Because of the strictly queue-like behaviour of the NM hook dispatcher the hooks that it wants to run are, in order Y (well, actually it's waiting for it to finish), Z, X, Y, Z. This can be problematic (and not only in this case... please save us systemd! :)).

So in this case I suspect that 30-i2p.sh is like Y due to tails-i2p start taking a long time with its long static sleep:s and wait_until:s. Am I on to something?

#38 Updated by kytv almost 4 years ago

anonym wrote:

anonym wrote:

Next,

[...]

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

Ah, could this be related to NetworkManager having stray hooks from previous connections? Let's say we have NM hooks X, Y, Z on "interface up". We connect to some network and hook X runs and exits, and Y is running, and it runs for a long time for some reason. We then disconnect even before Y exits. Then we connect again. Because of the strictly queue-like behaviour of the NM hook dispatcher the hooks that it wants to run are, in order Y (well, actually it's waiting for it to finish), Z, X, Y, Z. This can be problematic (and not only in this case... please save us systemd! :)).

So in this case I suspect that 30-i2p.sh is like Y due to tails-i2p start taking a long time with its long static sleep:s and wait_until:s. Am I on to something?

I think that's about right.

The snapshot is taken just after the reseeding/bootstrapping completes. When we restore the snapshot the original tails-i2p will still be running, wait:ing_until I2P should be ready.

#39 Updated by kytv almost 4 years ago

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 almost 4 years ago

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

Changes made/pushed.

#41 Updated by anonym almost 4 years ago

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

#42 Updated by anonym almost 4 years ago

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

#43 Updated by anonym almost 4 years ago

  • Status changed from Fix committed to Resolved

Also available in: Atom PDF