Feature #6094

Test suite: background snapshot improvements

Added by Tails 2013-07-18 07:50:50 . Updated 2016-02-21 21:46:09 .

Status:
Resolved
Priority:
High
Assignee:
Category:
Test suite
Target version:
Tails_1.7
Start date:
Due date:
2015-10-15
% Done:

100%

Feature Branch:
test/6094-improved-snapshots
Type of work:
Code
Blueprint:

Starter:
0
Affected tool:
Deliverable for:
266

Description

While background snapshots are invaluable for speeding up the test suite and not hammering the Tor authorities excessively with complete bootstraps, they do introduce some ugly code overhead and confusion:

  • There’s the need of the I save the state ... step at the end of the Background, which is pretty hackish.
  • Each "skippable" step needs an next if ... line around the top of the step.
  • In the previous point I say "around" the top, because sometimes input from a step are to be saved in some global variable to be accessible for subsequent steps, and then those assignments should happen before the next if .... It’s pretty confusing.

It’d be great if we could tag steps skippable and skip them when appropriate using something like an AroundStep('skippable’)@ hook, but vanilla cucumber supports neither tagging of steps nor such a hook (cucumber-cpp does, though). OTOH, with all that we’d be unable to save any input from the skipped steps, which probably would require some interesting workarounds (maybe using real $global variables instead of class variables will do?).

So the question is if we really can improve the situation. An AfterBackground('snapshot’)@ hook would at least be nice for killing the I save the state ... step in features tagged @snapshot. Can this be implemented using formatters, e.g. like our custom {After,Before}Feature hooks in features/support/extra_hooks.rb?

Subtasks

Related issues

Related to Tails - Feature #8008: Optimize tests that need a persistent volume Resolved 2014-10-05 2016-01-15
Blocks Tails - Feature #5707: Test suite: split various checks feature Resolved
Blocks Tails - Feature #5847: Improve how we pass data between steps Confirmed
Blocks Tails - Feature #10237: Refactor and clean up the automated test suite Confirmed 2015-03-07
Blocks Tails - Feature #10258: Test that Browsers in Tails use the IE theme when Windows Camouflage is enabled Confirmed 2015-09-26
Blocks Tails - Feature #6302: Write MAC spoofing tests Resolved 2013-09-26

History

#1 Updated by intrigeri 2013-10-03 12:43:54

  • Category set to Test suite
  • Starter set to No

#2 Updated by BitingBird 2014-06-09 11:23:34

  • Subject changed from test suite: background snapshot improvements to Test suite: background snapshot improvements

#3 Updated by intrigeri 2014-10-20 16:02:45

  • Description updated

#4 Updated by anonym 2014-10-22 16:11:29

One way to approach this is to stop pretending that our scenarios are run on real hardware, and instead incorporate the exact VM actions as steps. This matches exactly how I do manual testing — I heavily rely on making snapshot to avoid waiting for Tails to reboot and any other state I’ve set up.

For instance, what we now call the “Background” would be a scenario of its own, and in the end it has a “Then I create the snapshot XXX”, where XXX is a name that describes the state properly. Scenarios depending on it would then simply start with “Given I restore the snapshot XXX”, and then proceed as usual. Here’s an example of torified_browser.feature:

  Scenario: Set up a snapshot for testing the Tor Browser
    Given a virtual machine
    And I capture all network traffic
    And I start the virtual machine
    And the virtual machine boots Tails
    And I log in to a new session
    And GNOME has started
    And Tor is ready
    And available upgrades have been checked
    And all notifications have disappeared
    And I create the snapshot "networked_boot_post_login"

  Scenario: The Tor Browser uses TBB's shared libraries
    Given I restore the snapshot "networked_boot_post_login"
    When I start the Tor Browser
    And the Tor Browser has started
    Then the Tor Browser uses all expected TBB shared libraries

[... similar for remaining scenarios ...]

Someone could follow these steps manually and get the same results as the automated test suite. Well, some steps would potentially be hidden: I guess we’ll still need some post-snapshot-restore hooks to deal with system time drift, and Tor’s connection being abruptly severed, and so on. We could make them into real steps, but I feel like that would be bloating.

It’s worth noting that the simplistic snapshots we use at the moment only takes into account RAM, and do not “branch” disk storage, but the latter is possible and I’ve tested it in our current automated test suite (@vm.domain.snapshot_create_xml() and @vm.domain.revert_to_snapshot(), plus some code to make appropriate XML files describing the snapshots, e.g. which disks to branch and how). That will be very useful for Feature #8008.

However, Feature #8008 calls for cross-feature snapshots, and certainly that’d be useful as an optimization for almost all other features as well. We currently delete snapshots after finishing a feature, but that’s easily fixed. The greater problem is that to use the above approach across features, we then would have to ensure that the scenario that creates the snapshot runs first. I.e. order we run the features in matters, and some features cannot be run in isolation. That sucks.

One straight-forward way to solve both problem is to make all the snapshot creation scenarios into a single step: “Then I create the snapshot XXX” that simply skips if the snapshot already exists. That step would look something like this like this:

Then /^I create the snapshot "(.*)"$/ do |snapshot|
  next if File.exists? snapshot
  case snapshot
  when "networked_boot_post_login"
    step "a virtual machine"
    step "I capture all network traffic"
    step "I start the virtual machine"
    [... rest of the steps from the snapshot creation scenario above ...]
    step "all notifications have disappeared"
    @vm.save_all_vm_state_to_snapshot(snapshot)
  when "some_other_snapshot"
    step "a virtual machine"
    [...]
  end
end

We could even be smart and recursively call step "create the snapshot ..." at all branching points, although all that would have to be arranged and thought through manually. I haven’t looked at exactly how the snapshots work, but if the complete RAM state is saved (and not only the diff against the previous snapshot) for each snapshot, the disk usage may get too large so we don’t want to make this branching too excessive.

An IMHO massive problem, however, is that this approach hides a large part of what’s going on from the scenarios. That makes me much less enthusiastic with this approach. We could perhaps write to stderr which steps are being run. Hmm.

Another issue with everything I’ve said so far is that since the name of the snapshot essentially has to parametrize the VM state it contains, the names will get excessively long and complicated.

It may be worth investigating if tags can make this prettier and more transparent. Or perhaps some facility that records which steps have been run, and somehow determinines when to save and restore backups based on such recordings.

That’s some random thoughts for now.

#5 Updated by anonym 2014-10-22 16:11:53

  • related to Feature #8008: Optimize tests that need a persistent volume added

#6 Updated by anonym 2014-11-06 12:36:30

I just found out that Cucumber has a pull-request for adding a BeforeStep hook. That will eliminate the need for the next if ... part of each step if there is a way to tell cucumber to automatically pass the step too.

Combined with the already existing AfterStep, we may have some interesting options for some elaborate, automagic snapshot saving/restoring scheme.

#7 Updated by anonym 2015-01-09 13:48:25

  • blocks Feature #5707: Test suite: split various checks feature added

#8 Updated by intrigeri 2015-01-09 15:59:25

anonym wrote:
> I just found out that Cucumber has a pull-request for adding a BeforeStep hook.

No progress since mid-October, 2014.

#9 Updated by intrigeri 2015-01-09 16:30:37

In the high-level view (*.features), Then /^I create the snapshot "(.*)"$/ do |snapshot| should instead be Then /^I reach the "(.*)" checkpoint$/ do |checkpoint|. It still would do the same as described in note 4, that is restoring a snapshot, possibly creating it first.

Regarding added complexity, assuming this stuff allows us to drop the I save the state and next if noise, it seems that the end result won’t be any worse than what we have now, and maybe even better.

Regarding hidden steps making the test suite hard to understand, we could wrap or override step to output whatever it’s doing.

Regarding recursively calling the aforementioned step, I think it would be good to have, but even without it, the solution described in note 4 would already be a great improvement over the current state of things, in terms of robustness, performance and DRYness, so IMO the recursive part is better kept out of the first iteration.

#10 Updated by intrigeri 2015-01-09 16:31:46

  • Assignee set to anonym
  • Target version set to Sustainability_M1
  • Type of work changed from Research to Code

#11 Updated by anonym 2015-01-10 06:40:20

  • Target version changed from Sustainability_M1 to Tails_1.8

#12 Updated by anonym 2015-01-10 17:16:32

  • blocks #8668 added

#13 Updated by anonym 2015-06-03 17:07:45

  • Feature Branch set to test/wip-improved-snapshots

So the test/wip-improved-snapshots branch fixes these issues by making it a bit more explicit that we use snapshots. The snapshots are not “locked” to be defined in the background, and can be used in any feature (and between them — they’re only ever created once per test suite run). They’re unfortunately a bit hidden (in features/step_definitions/snapshots.rb) but the steps that restore them have pretty intuitive names. Also, the used of @skip_steps_while_restoring_backgroundu is completely eliminated.

Performance comparison on isotester1.lizard:

With the stable branch:

Failing Scenarios:
cucumber features/i2p.feature:18 # Scenario: I2P is enabled when the "i2p" boot parameter is added
cucumber features/torified_browsing.feature:19 # Scenario: The Tor Browser directory is usable
cucumber features/torified_gnupg.feature:48 # Scenario: Syncing OpenPGP keys using Seahorse started from the Tails OpenPGP Applet should work and be done over Tor.
cucumber features/torified_misc.feature:20 # Scenario: wget(1) should work for HTTPS and go through Tor.
cucumber features/totem.feature:28 # Scenario: Watching a WebM video over HTTPS, with and without the command-line

183 scenarios (5 failed, 178 passed)
1747 steps (5 failed, 8 skipped, 1734 passed)
425m38.506s


With the test/wip-improved-snapshots branch:

Failing Scenarios:
cucumber features/electrum.feature:13 # Scenario: Using a persistent Electrum configuration
cucumber features/torified_browsing.feature:39 # Scenario: Watching a WebM video

176 scenarios (2 failed, 174 passed)
1249 steps (2 failed, 11 skipped, 1236 passed)
306m43.259s

None of the failed steps in each of the two runs take a very long time upon failure (and not much was skipped afterwards), so I think the results should be comparable. So this branch introduces a ~30% reduction in run speed. Also it’s now a lot cheaper to test persistence (in separate feature) which is good news for Feature #8008.

#14 Updated by intrigeri 2015-06-03 17:28:51

Amazing, congrats!

#15 Updated by anonym 2015-07-15 19:03:21

I’ve pushed a version with devel as of yesterday (2015-07-15) merged in and adapted all new stuff to the new snapshot system (I’ve run two full run successfully too). Also, the --keep-snapshots has been restored, which I hope will make intrigeri happy. :)

#16 Updated by BitingBird 2015-07-29 09:51:26

  • Status changed from Confirmed to In Progress
  • % Done changed from 0 to 20

#19 Updated by sajolida 2015-08-05 09:53:27

  • Due date set to 2015-10-15

#20 Updated by intrigeri 2015-08-07 03:55:13

anonym, how does this relate to Bug #5571 and subtasks?

#21 Updated by anonym 2015-08-07 05:14:48

intrigeri wrote:
> anonym, how does this relate to Bug #5571 and subtasks?

Unless Bug #5571 is solved, scenarios that require a filesystem share cannot use snapshots at all. Well, one could restore the snapshot, reboot Tails and get filesystem shares, but that will only be relevant if the important thing in the snapshot persist a reboot, so USB install and persistence related stuff only.

#22 Updated by anonym 2015-08-07 05:56:31

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

#25 Updated by intrigeri 2015-08-08 02:14:27

> Unless Bug #5571 is solved, scenarios that require a filesystem share cannot use snapshots at all. Well, one could restore the snapshot, reboot Tails and get filesystem shares, but that will only be relevant if the important thing in the snapshot persist a reboot, so USB install and persistence related stuff only.

OK. And then, Bug #5571 is not a blocker of this ticket, strictly speaking: the WIP snapshots branch brings us substantial performance improvements even without Bug #5571 being solved first.

#26 Updated by anonym 2015-08-08 06:17:18

  • blocks Feature #5847: Improve how we pass data between steps added

#27 Updated by anonym 2015-08-08 06:18:06

Bonus: this branch will resolve Feature #5847.

#28 Updated by anonym 2015-08-13 11:44:41

  • Feature Branch changed from test/wip-improved-snapshots to test/6094-improved-snapshots

#29 Updated by anonym 2015-08-31 08:08:08

  • Assignee changed from anonym to kytv
  • % Done changed from 20 to 50
  • QA Check set to Ready for QA

Please test, review and merge this branch.

The magic in this branch really happens in the Tails has booted ... steps that are dynamically generated in features/step_definitions/snapshots.rb. These signify that we will restore the state from a snapshot (or generate it from scratch) and drops the need for the crazy I save the state so the background can be restored next scenario step, and the next if ... in the beginning of each step definition. We still have the old I start Tails ... steps, since they are needed for scenarios where we cannot use snapshots (e.g. when filesystem shares are needed, due to Bug #5571).

Beyond some mechanical variable renames (and removal all instances of next if ... in the beginning of step definitions) not much has changed in the step definitions. A lot has changed in the .feature files, though, so that has to be reviewed carefully: some have been split; some scenarios have been reworked to fit this new snapshot system better. Hence it’s very important that the reviewer will check that no test coverage is lost in this branch, i.e. that we test at least as much as we did before. I would suggest looking at the git diff instead of the Git history.

Also note that there’s an XXX comment in persistence.feature for how guestfs vs qcow2 internal snapshots. I’ll try to work this out, but from my extensive testing it works, it’s just that I don’t understand why. :)

#30 Updated by intrigeri 2015-09-01 04:38:50

> Please test, review and merge this branch.

Perhaps it would be worth adding some bits to the “how to write tests” doc, e.g. listing and describing the standard checkpoints (shared snapshots) we have?

#31 Updated by kytv 2015-09-04 07:29:36

I’ve done a quick review of this branch (and many, many runs) and I’m very impressed. That, however, can probably go without saying.

I found a couple of minor comment issues which I’ll provide a patch for. I’ll also do more in-depth reviewing in the next few days.

(I ported my I2P tests which use temporary snapshots to this system and even that was easy. I ♥ this branch.)

#32 Updated by kytv 2015-09-04 07:51:03

My minor change:

diff --git a/features/support/helpers/vm_helper.rb b/features/support/helpers/vm_helper.rb
index 09913a5..71e2940 100644
--- a/features/support/helpers/vm_helper.rb
+++ b/features/support/helpers/vm_helper.rb
@@ -503,9 +503,9 @@ EOF
     # If we have no qcow2 disk device, we'll use "memory state"
     # snapshots, and if we have at least one qcow2 disk device, we'll
     # use internal "system checkpoint" (memory + disks) snapshots. We
-    # have to do this since internal snapshots doesn't work when no
+    # have to do this since internal snapshots don't work when no
     # such disk is available. We can do this with external snapshots,
-    # which better in many ways, but libvirt doesn't know how to
+    # which are better in many ways, but libvirt doesn't know how to
     # restore (revert back to) them yet.
     # WARNING: If only transient disks, i.e. disks that were plugged
     # after starting the domain, are used then the memory state will

#33 Updated by kytv 2015-09-06 04:06:09

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

I don’t know if this is a new regression from this branch or a separate bug which deserves its own ticket. I’ll explain what I see and you might know immediately.

While testing a set of my tests, the scenario failed. I had --pause-on-fail enabled and pressed CTRL+C to abort, with the goal being to make a change to the ruby and re-run the test.

In doing so I had the following result:

# ./run_test_suite  --keep-snapshots --pause-on-fail --view --debug --iso ../isos/tails-i386-6094-with-new-i2p-1.6-20150906.iso  --log-to-file=i2p.log  -- features/i2p.feature
Virtual X framebuffer started on display :1
VNC server running on: localhost:5900
[info] Sikuli vision engine loaded.
Testing ISO image: tails-i386-6094-with-new-i2p-1.6-20150906.iso
Call to virNetworkCreate failed: Unable to create bridge virbr10: File exists (Libvirt::Error)
/home/kytv/tails/features/support/helpers/vm_helper.rb:40:in `create'
/home/kytv/tails/features/support/helpers/vm_helper.rb:40:in `update'
/home/kytv/tails/features/support/helpers/vm_helper.rb:20:in `initialize'
/home/kytv/tails/features/support/hooks.rb:63:in `new'
/home/kytv/tails/features/support/hooks.rb:63:in `block in <top (required)>'
/home/kytv/tails/features/support/extra_hooks.rb:35:in `call'
/home/kytv/tails/features/support/extra_hooks.rb:35:in `invoke'
/home/kytv/tails/features/support/extra_hooks.rb:68:in `block in before_feature'
/home/kytv/tails/features/support/extra_hooks.rb:67:in `each'
/home/kytv/tails/features/support/extra_hooks.rb:67:in `before_feature'
/usr/lib/ruby/vendor_ruby/cucumber/ast/tree_walker.rb:181:in `block in send_to_all'
/usr/lib/ruby/vendor_ruby/cucumber/ast/tree_walker.rb:179:in `each'
/usr/lib/ruby/vendor_ruby/cucumber/ast/tree_walker.rb:179:in `send_to_all'
/usr/lib/ruby/vendor_ruby/cucumber/ast/tree_walker.rb:169:in `broadcast'
/usr/lib/ruby/vendor_ruby/cucumber/ast/tree_walker.rb:26:in `visit_feature'
/usr/lib/ruby/vendor_ruby/cucumber/ast/features.rb:28:in `block in accept'
/usr/lib/ruby/vendor_ruby/cucumber/ast/features.rb:17:in `each'
/usr/lib/ruby/vendor_ruby/cucumber/ast/features.rb:17:in `each'
/usr/lib/ruby/vendor_ruby/cucumber/ast/features.rb:27:in `accept'
/usr/lib/ruby/vendor_ruby/cucumber/ast/tree_walker.rb:21:in `block in visit_features'
/usr/lib/ruby/vendor_ruby/cucumber/ast/tree_walker.rb:170:in `broadcast'
/usr/lib/ruby/vendor_ruby/cucumber/ast/tree_walker.rb:20:in `visit_features'
/usr/lib/ruby/vendor_ruby/cucumber/runtime.rb:49:in `run!'
/usr/lib/ruby/vendor_ruby/cucumber/cli/main.rb:42:in `execute!'
/usr/bin/cucumber:13:in `<main>'
/home/kytv/tails/features/support/hooks.rb:184:in `block in <top (required)>': undefined method `destroy_and_undefine' for nil:NilClass (NoMethodError)

The ISO & branch for Feature #6306 are based off of Feature #6094 with Feature #9516 merged in.

I was able to re-run the test suite with ip link del dev virbr10.

Reassigning to anonym but I’ll still conduct more in-depth reviews of this branch.

#34 Updated by kytv 2015-09-06 04:59:38

And another time after pressing CTRL+C, same tests, same branch, same iso:

Testing ISO image: tails-i386-6094-with-new-i2p-1.6-20150906.iso
Call to virNetworkCreate failed: internal error: Child process (/sbin/ip addr add fc00::1/7 dev virbr10) unexpected exit status 2: RTNETLINK answers: Permission denied
 (Libvirt::Error)

#35 Updated by anonym 2015-09-08 08:17:23

kytv wrote:
> I don’t know if this is a new regression from this branch or a separate bug which deserves its own ticket. I’ll explain what I see and you might know immediately.
>
> While testing a set of my tests, the scenario failed. I had --pause-on-fail enabled and pressed CTRL+C to abort, with the goal being to make a change to the ruby and re-run the test.
>
> In doing so I had the following result:
>
> […]

I cannot reproduce it. This is exactly what I do, based on my understanding of the problem:

1. I checkout a clean test/6094-improved-snapshots

2. I create this feature:

@product
Feature: Failing, mostly

  Background:
    Given Tails has booted from DVD and logged in and the network is connected

  Scenario: success1
    When success

  Scenario: failure1
    When failure

  Scenario: success2
    When success

  Scenario: failure2
    When failure

  Scenario: success3
    When success

  Scenario: failure3
    When failure
When /^failure$/ do
  raise
end

When /^success$/ do
  next
end

3. Then I run the test suite like this:

./run_test_suite --keep-snapshots --pause-on-fail --view --debug --iso tails-i386-1.5.1.iso -- features/fail.feature

4. In various failing scenarios (when primpted to press ENTER to resume) I instead press Ctrl+C.

5. Goto 3.

I haven’t seen a single instance of the issue you describe. Could you elaborate what you did, so I can try it exactly (and preferably retry it so you know it’s still an issue).

Also, let’s compare libvirt environment:

$ dpkg -l libvirt* | awk '/^ii/ { print $2, $3 }'
libvirt-bin 1.2.9-9
libvirt-clients 1.2.9-9
libvirt-daemon 1.2.9-9
libvirt-daemon-system 1.2.9-9
libvirt-dev 1.2.9-9
libvirt-glib-1.0-0 0.1.9-4
libvirt-ruby 0.4.0-1
libvirt0 1.2.9-9

> The ISO & branch for Feature #6306 are based off of Feature #6094 with Feature #9516 merged in.

I doubt Feature #9516 has anything to do with it (it introduces a one line change). I haven’t seen your code for Feature #6306, but I also doubt it would change anything too, unless it touches too many things outside step definitions and .feature files. Just in case, perhaps you can push your WIP branch of Feature #6306 so I can have a look?

#36 Updated by kytv 2015-09-14 14:01:33

After stable was merged in I now get this failure:

  Scenario: Using a persistent Electrum configuration                                                         # features/electrum.feature:13
    Checkpoint: Tails has booted without network from a USB drive without a persistent partition and stopped at Tails Greeter's login screen
      Given Tails has booted from DVD without network and logged in
      And I create a 4 GiB disk named "current"
    Step failed while creating checkpoint: I plug USB drive "current"
    Given Tails has booted without network from a USB drive with a persistent partition enabled and logged in # features/step_definitions/snapshots.rb:194
      Tails is not running (RuntimeError)
      ./features/step_definitions/common_steps.rb:107:in `/^drive "([^"]+)" is detected by Tails$/'
      ./features/step_definitions/common_steps.rb:102:in `/^I plug (.+) drive "([^"]+)"$/'
      ./features/step_definitions/snapshots.rb:177:in `block in reach_checkpoint'
      ./features/step_definitions/snapshots.rb:175:in `each'
      ./features/step_definitions/snapshots.rb:175:in `reach_checkpoint'
      ./features/step_definitions/snapshots.rb:162:in `reach_checkpoint'
      ./features/step_definitions/snapshots.rb:162:in `reach_checkpoint'
      ./features/step_definitions/snapshots.rb:195:in `/^Tails has booted without network from a USB drive with a persistent partition enabled and logged in$/'
      features/electrum.feature:14:in `Given Tails has booted without network from a USB drive with a persistent partition enabled and logged in'

#37 Updated by kytv 2015-09-14 14:05:20

anonym wrote:
> kytv wrote:

>
> I cannot reproduce it.

I cannot reliably reproduce it, it just “happens” sometimes. Still trying to find some reliable way of reproducing it.

> Also, let’s compare libvirt environment:
> […]

# dpkg -l libvirt* | awk '/^ii/ { print $2, $3 }'
libvirt-bin 1.2.9-9+deb8u1
libvirt-clients 1.2.9-9+deb8u1
libvirt-daemon 1.2.9-9+deb8u1
libvirt-daemon-system 1.2.9-9+deb8u1
libvirt-dev 1.2.9-9+deb8u1
libvirt-glib-1.0-0 0.1.9-4
libvirt0 1.2.9-9+deb8u1

#38 Updated by anonym 2015-09-16 14:56:25

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

Returning the ticket to you, then. I never got this, and I haven’t seen it on any isotester* (although I don’t Ctrl+C very often), so perhaps we can not block on this? In particular, this branch is pretty sorely needed for the Jenkins deployment, and Ctrl+C won’t be an issue there, and --keep-snapshots won’t be used. If you (or any one else) see it again, lry’s open a new ticket.

#39 Updated by anonym 2015-09-16 16:45:16

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

This won’t be merged in time for 1.6, quite clearly. Please hold off and start reviewing preferably as early as possible after the 1.6 release — focus on the other test suite 1.6 tickets instead.

#40 Updated by kytv 2015-09-17 01:48:26

The windows_camoflage feature fails as of revision 97547d3057b50c7d17e29182002a20c2df04a974

# ./run_test_suite   --view --iso ../isos/tails-i386-feature_i2p-0.9.22-1.6-20150914.iso  -- --format pretty --format debug --out /tmp/debug.log features/windows_camouflage.feature
Virtual X framebuffer started on display :1
VNC server running on: localhost:5900
[info] Sikuli vision engine loaded.
Testing ISO image: tails-i386-feature_i2p-0.9.22-1.6-20150914.iso
Using old ISO image: tails-i386-feature_i2p-0.9.22-1.6-20150914.iso
@product
Feature: Microsoft Windows Camouflage
  As a Tails user
  when I select the Microsoft Windows Camouflage in Tails Greeter
  I should be presented with a Microsoft Windows like environment

  Background:                                                                                     # features/windows_camouflage.feature:7
    Checkpoint: Tails has booted from DVD without network and stopped at Tails Greeter's login screen
      Given the network is unplugged
      And I start the computer
      And the computer boots Tails
    Checkpoint: Tails has booted from DVD without network and logged in with windows camouflage enabled
      Given Tails has booted from DVD without network and stopped at Tails Greeter's login screen
      And I enable more Tails Greeter options
      And I enable Microsoft Windows camouflage
      And I enable the specific Tor configuration option
      And I log in to a new session
      And Tails Greeter has dealt with the sudo password
      And the Tails desktop is ready
      And all notifications have disappeared
    Given Tails has booted from DVD without network and logged in with windows camouflage enabled # features/step_definitions/snapshots.rb:194

  Scenario: I should be presented with a Microsoft Windows like desktop                           # features/windows_camouflage.feature:10
    Then I see "WindowsDesktop.png" after at most 10 seconds                                      # features/step_definitions/common_steps.rb:374
    And I see "WindowsStartButton.png" after at most 10 seconds                                   # features/step_definitions/common_steps.rb:374
    And I see "WindowsSysTrayGpgApplet.png" after at most 10 seconds                              # features/step_definitions/common_steps.rb:374
    And I see "WindowsSysTrayKeyboard.png" after at most 10 seconds                               # features/step_definitions/common_steps.rb:374
    And I see "WindowsSysTraySound.png" after at most 10 seconds                                  # features/step_definitions/common_steps.rb:374

  Scenario: Windows should appear like those in Microsoft Windows                                 # features/windows_camouflage.feature:17
    When the network is plugged                                                                   # features/step_definitions/common_steps.rb:113
    And Tor is ready                                                                              # features/step_definitions/common_steps.rb:301
      try_for() timeout expired (Timeout::Error)
      ./features/support/helpers/misc_helpers.rb:74:in `rescue in try_for'
      ./features/support/helpers/misc_helpers.rb:24:in `try_for'
      ./features/support/helpers/misc_helpers.rb:78:in `wait_until_tor_is_working'
      ./features/step_definitions/common_steps.rb:307:in `/^Tor has built a circuit$/'
      ./features/step_definitions/common_steps.rb:302:in `/^Tor is ready$/'
      features/windows_camouflage.feature:19:in `And Tor is ready'
    And all notifications have disappeared                                                        # features/step_definitions/common_steps.rb:364
    And available upgrades have been checked                                                      # features/step_definitions/common_steps.rb:316
    And I start the Tor Browser                                                                   # features/step_definitions/common_steps.rb:476
    Then I see "WindowsTorBrowserWindow.png" after at most 120 seconds                            # features/step_definitions/common_steps.rb:374
    And I see "WindowsTorBrowserTaskBar.png" after at most 10 seconds                             # features/step_definitions/common_steps.rb:374
    And I see "WindowsWindowButtons.png" after at most 10 seconds                                 # features/step_definitions/common_steps.rb:374
Scenario failed at time 00:06:22
Took screenshot "/tmp/TailsToaster/windows_camouflage-2015-09-16T15:57:27+00:00.png"

  Scenario: The panel menu should look like Microsoft Windows's start menu                        # features/windows_camouflage.feature:27
    When I click the start menu                                                                   # features/step_definitions/windows_camouflage.rb:6
    Then I see "WindowsStartMenu.png" after at most 10 seconds                                    # features/step_definitions/common_steps.rb:374

Failing Scenarios:
cucumber features/windows_camouflage.feature:17 # Scenario: Windows should appear like those in Microsoft Windows

3 scenarios (1 failed, 2 passed)
18 steps (1 failed, 6 skipped, 11 passed)
6m26.101s
invalid option: --format
Test::Unit automatic runner.
Usage: /usr/bin/cucumber [options] [-- untouched arguments]
    -r, --runner=RUNNER              Use the given RUNNER.
                                     (c[onsole], e[macs], x[ml])
        --collector=COLLECTOR        Use the given COLLECTOR.
                                     (de[scendant], di[r], l[oad], o[bject]_space)
    -n, --name=NAME                  Runs tests matching NAME.
                                     (patterns may be used).
        --ignore-name=NAME           Ignores tests matching NAME.
                                     (patterns may be used).
    -t, --testcase=TESTCASE          Runs tests in TestCases matching TESTCASE.
                                     (patterns may be used).
        --ignore-testcase=TESTCASE   Ignores tests in TestCases matching TESTCASE.
                                     (patterns may be used).
        --location=LOCATION          Runs tests that defined in LOCATION.
                                     LOCATION is one of PATH:LINE, PATH or LINE
        --attribute=EXPRESSION       Runs tests that matches EXPRESSION.
                                     EXPRESSION is evaluated as Ruby's expression.
                                     Test attribute name can be used with no receiver in EXPRESSION.
                                     EXPRESSION examples:
                                       !slow
                                       tag == 'important' and !slow
        --[no-]priority-mode         Runs some tests based on their priority.
        --default-priority=PRIORITY  Uses PRIORITY as default priority
                                     (h[igh], i[mportant], l[ow], m[ust], ne[ver], no[rmal])
    -I, --load-path=DIR[:DIR...]     Appends directory list to $LOAD_PATH.
        --color-scheme=SCHEME        Use SCHEME as color scheme.
                                     (d[efault])
        --config=FILE                Use YAML fomat FILE content as configuration file.
        --order=ORDER                Run tests in a test case in ORDER order.
                                     (a[lphabetic], d[efined], r[andom])
        --max-diff-target-string-size=SIZE
                                     Shows diff if both expected result string size and actual result string size are less than or equal SIZE in bytes.
                                     (1000)
    -v, --verbose=[LEVEL]            Set the output level (default is verbose).
                                     (important-only, n[ormal], p[rogress], s[ilent], v[erbose])
        --[no-]use-color=[auto]      Uses color output
                                     (default is auto)
        --progress-row-max=MAX       Uses MAX as max terminal width for progress mark
                                     (default is auto)
        --no-show-detail-immediately Shows not passed test details immediately.
                                     (default is yes)
        --output-file-descriptor=FD  Outputs to file descriptor FD
        --                           Stop processing options so that the
                                     remaining options will be passed to the
                                     test.
    -h, --help                       Display this help.

Deprecated options:
        --console                    Console runner (use --runner).

#41 Updated by kytv 2015-09-17 01:56:43

anonym wrote:
> Returning the ticket to you, then. I never got this, and I haven’t seen it on any isotester* (although I don’t Ctrl+C very often), so perhaps we can not block on this? In particular, this branch is pretty sorely needed for the Jenkins deployment, and Ctrl+C won’t be an issue there, and --keep-snapshots won’t be used. If you (or any one else) see it again, lry’s open a new ticket.

This problem is almost certainly caused by something else. I saw it while running the test suite in another branch (Feature #6302) and there was no CTRL+C action, so that was just a coincidence. I opened Bug #10211 for this totally unrelated problem.

Now there’s just the problem that I reported in Feature #6094#note-40 which is probably caused by a bad merge.

#42 Updated by kytv 2015-09-17 02:05:10

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

#43 Updated by anonym 2015-09-17 05:13:27

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

kytv wrote:
> This problem is almost certainly caused by something else. I saw it while running the test suite in another branch (Feature #6302) and there was no CTRL+C action, so that was just a coincidence. I opened Bug #10211 for this totally unrelated problem.

Great! Well, I think this branch may be the cause, because with --keep-snapshots the domain and network is not undefined after the test suite finishes, and when you re-run it with --keep-snapshots it tries to reuse that “left-over” domain and network. I’ll add this info to Bug #10211.

> Now there’s just the problem that I reported in Feature #6094#note-40 which is probably caused by a bad merge.

Fixed in commit:84b8f5e.

#44 Updated by kytv 2015-09-21 17:31:12

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

At the end of a scenario is the following output:

invalid option: --format
Test::Unit automatic runner.
Usage: /usr/bin/cucumber [options] [-- untouched arguments]
    -r, --runner=RUNNER              Use the given RUNNER.
                                     (c[onsole], e[macs], x[ml])
        --collector=COLLECTOR        Use the given COLLECTOR.
                                     (de[scendant], di[r], l[oad], o[bject]_space)
    -n, --name=NAME                  Runs tests matching NAME.
                                     (patterns may be used).
        --ignore-name=NAME           Ignores tests matching NAME.
                                     (patterns may be used).
    -t, --testcase=TESTCASE          Runs tests in TestCases matching TESTCASE.
                                     (patterns may be used).
        --ignore-testcase=TESTCASE   Ignores tests in TestCases matching TESTCASE.
                                     (patterns may be used).
        --location=LOCATION          Runs tests that defined in LOCATION.
                                     LOCATION is one of PATH:LINE, PATH or LINE
        --attribute=EXPRESSION       Runs tests that matches EXPRESSION.
                                     EXPRESSION is evaluated as Ruby's expression.
                                     Test attribute name can be used with no receiver in EXPRESSION.
                                     EXPRESSION examples:
                                       !slow
                                       tag == 'important' and !slow
        --[no-]priority-mode         Runs some tests based on their priority.
        --default-priority=PRIORITY  Uses PRIORITY as default priority
                                     (h[igh], i[mportant], l[ow], m[ust], ne[ver], no[rmal])
    -I, --load-path=DIR[:DIR...]     Appends directory list to $LOAD_PATH.
        --color-scheme=SCHEME        Use SCHEME as color scheme.
                                     (d[efault])
        --config=FILE                Use YAML fomat FILE content as configuration file.
        --order=ORDER                Run tests in a test case in ORDER order.
                                     (a[lphabetic], d[efined], r[andom])
        --max-diff-target-string-size=SIZE
                                     Shows diff if both expected result string size and actual result string size are less than or equal SIZE in bytes.
                                     (1000)
    -v, --verbose=[LEVEL]            Set the output level (default is verbose).
                                     (important-only, n[ormal], p[rogress], s[ilent], v[erbose])
        --[no-]use-color=[auto]      Uses color output
                                     (default is auto)
        --progress-row-max=MAX       Uses MAX as max terminal width for progress mark
                                     (default is auto)
        --no-show-detail-immediately Shows not passed test details immediately.
                                     (default is yes)
        --output-file-descriptor=FD  Outputs to file descriptor FD
        --                           Stop processing options so that the
                                     remaining options will be passed to the
                                     test.
    -h, --help                       Display this help.

Deprecated options:
        --console                    Console runner (use --runner).

I’ve not been able to figure out why.

#45 Updated by anonym 2015-09-23 09:10:35

  • blocks Feature #10237: Refactor and clean up the automated test suite added

#46 Updated by anonym 2015-09-25 05:40:07

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

kytv wrote:
> At the end of a scenario is the following output:

Fixed in commit commit:73f66d0. The comment added should explain all:

# Test::Unit adds an at_exit hook which, among other things, consumes
# the command-line arguments that were intended for cucumber. If
# e.g. `--format` was passed it will throw an error since it's not a
# valid option for Test::Unit, and it throwing an error at this time
# (at_exit) will make Cucumber think it failed and consequently exit
# with an error. Fooling Test::Unit that this hook has already run
# works around this craziness.

#47 Updated by intrigeri 2015-09-25 14:42:54

First part of my initial review:

  • Perhaps some “how to write tests” doc update would be useful? E.g. that doc still mentions skip_steps_while_restoring_background, which feels wrong.
  • “Tails has booted from DVD and logged in with” <= I think there’s a grammar bug in here.
  • LIBVIRT_DOMAIN_NAME, LIBVIRT_DOMAIN_UUID etc. look more like internal/private constants (suitable for features/support/env.rb maybe), than anything we should expose in a public configuration interface, no?
  • In electrum.feature a given step was removed, and then And I start Tails from USB drive "current" and I login with persistence enabled becomes a then while I think it should be a when.
  • Typo in “can be used in all languges”
  • A few newly introduced features have “As a” and “I want” but miss the “In order to” (or similar) step that is generally used to express the usefulness of the feature. In our current main usage of cucumber that’s certainly no big deal, but if we want to start using it for other purposes some day (at least I would very much like) then it would be useful to try and convey such messages via Gherkin. Maybe I can propose patches for that if you wish.
  • “I want use Tails persistence feature” seems to be missing a “to”.
  • In “Scenario: Using a persistent Electrum configuration”, I’m not sure why we use a checkpoint that has networking disabled, and then plug it in. Same in “Scenario: Using a persistent Pidgin configuration”.
  • In most features, when the Background would become a single Given step, then that step has been moved to the scenarios, and the background removed. I’m not 100% sure yet but so far I like it. It seems that the opposite design decision was made in ssh.feature, so I wanted to point it out in case it was an oversight.
  • There’s trailing whitespace in features/step_definitions/snapshots.rb.
  • In step_regex = Regexp.new(...), the arbitrary text that’s turned into a regexp should see its regexp special chars escaped, no?
  • I’ve not finished my review but I didn’t see if/where the removed “Then Pidgin has the expected accounts configured with random nicknames” has been re-added.

#48 Updated by intrigeri 2015-09-25 14:43:29

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

#49 Updated by kytv 2015-09-26 06:06:06

  • blocks Feature #10258: Test that Browsers in Tails use the IE theme when Windows Camouflage is enabled added

#50 Updated by intrigeri 2015-09-26 15:52:40

  • While reading save_snapshot it’s easy to believe that when internal_snapshot = false we’re doing the opposite of “internal” snapshots, that is “external” snapshots. Apparently this is not the case. A comment, or improving variable naming, would fix that. Note that later on in the same file I see the nomenclature is “ram_only_snapshot” vs. “internal_snapshot”. Reuse it consistently?
  • I’m not convinced by the move of stuff to features/time_syncing_bridges.feature. Can we merge it back into time_syncing.feature? If needed, it might be that one can define a Background in the middle of a feature and see it applied only to the scenarios that follow.
  • Probably off-topic, but it looks like the time syncing and Tor bridges tests would be good candidates for DRY’ing feature files a bit with scenario outlines. Want a ticket about it as part of the test suite refactoring / cleanup project?
  • There’s at least one occurence of “two Given steps in a row” caused by semi-mechanical adjustements.
  • “Scenario: Copying video files to a persistence and making sure that they persist”’s name seems to not match what it currently does.
  • The simple “Scenario: Installing Tails to a pristine USB drive” has apparently disappeared, and commit:b151827 doesn’t really explain why. I guess that’s because this work is done somewhere else in a hidden way. If this guess is right, then I don’t like it: that scenario describes one of the most common use cases of Tails Installer, and one purpose of Gherkin features is to document how stuff should work, so IMO this specific behaviour is totally worth being explicitly documented in Gherkin. Similarly, it looks like “Scenario: Booting Tails from a USB drive without a persistent partition and creating one” has disappeared and IMO at least some part of it should come back somehow.
  • In “Scenario: Booting Tails from a USB drive in UEFI mode” the Given/Then/Given/When sequence feels strange.
  • I see “XXX: The next step succeeds […]” and a few “XXX: how does guestfs work vs snapshots?” => please create tickets if you think they can be useful, or something.
  • Typo in “including ones makred”.

And that’s all for this first full review of the diff :)

#51 Updated by sajolida 2015-10-01 07:23:43

  • Priority changed from Normal to High

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

#52 Updated by intrigeri 2015-10-01 08:57:26

  • Priority changed from High to Normal

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

The research was due for July 15, and it’s been completed and reported about already. Now we’re talking about completing a huge amount of bonus work, that is the implementation, which wasn’t promised to anyone. Sorry this ticket and the last report don’t make it clear. I think we should have closed this ticket as resolved and created a new one for the implementation.

#53 Updated by anonym 2015-10-04 02:02:07

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

> Perhaps some “how to write tests” doc update would be useful? E.g. that doc still mentions skip_steps_while_restoring_background, which feels wrong.

commit:381d61a

> “Tails has booted from DVD and logged in with” <= I think there’s a grammar bug in here.

Care to elaborate? I don’t quite get it. “and logged in” => “is logged in”? It definitely isn’t the most perfect sentence construction; suggestions for improvements are welcome!

> LIBVIRT_DOMAIN_NAME, LIBVIRT_DOMAIN_UUID etc. look more like
> internal/private constants (suitable for features/support/env.rb
> maybe), than anything we should expose in a public configuration
> interface, no?

I originally made them configurable like this to test how running multiple instances of the test suite under the same host would work out. Spoiler: there’s some more work left to do that! :)

Any way, I personally found this feature quite useful; when developing this branch I used it to re-configured LIBVIRT_{DOMAIN,NETWORK}_NAME differently to have less issues when switching to/from this branch, so, as a test suite developer, I quite like having this flexibility. It will be useful in future branches that break VM compatibility with the base branches, or similar.

Of course, changing them in such a way as I needed is only one commit away if we make them into constants. Whatever. If you feel like they should go, I’ll make them go.

So what do you think?

> In electrum.feature a given step was removed, and then And I start Tails from USB drive “current” and I login with persistence enabled becomes a then while I think it should be a when.

commit:82b78d1

> Typo in “can be used in all languges”

commit:0020aad

> A few newly introduced features have “As a” and “I want” but miss the “In order to” (or similar) step that is generally used to express the usefulness of the feature. In our current main usage of cucumber that’s certainly no big deal, but if we want to start using it for other purposes some day (at least I would very much like) then it would be useful to try and convey such messages via Gherkin. Maybe I can propose patches for that if you wish.

Right. Let’s focus on this in another ticket, since it’s basically fucked in every feature. I filed Feature #10329 for this.

> “I want use Tails persistence feature” seems to be missing a “to”.

comment:fe7edf1

> In “Scenario: Using a persistent Electrum configuration”, I’m not sure why we use a checkpoint that has networking disabled, and then plug it in. Same in “Scenario: Using a persistent Pidgin configuration”.

There is no checkpoint with persistence + networking. It seemed to me that the most common case for persistence is without networking, and timewise it’s pretty cheap (we will have to wait for Tor to working in either case). Adding a peristence + networking checkpoint it trivial, but will cost us ~900 MiB disk space in TMPDIR. If you think it’s worth it vs the isotester specs, I’ll go ahead an add it.

> In most features, when the Background would become a single Given step, then that step has been moved to the scenarios, and the background removed. I’m not 100% sure yet but so far I like it. It seems that the opposite design decision was made in ssh.feature, so I wanted to point it out in case it was an oversight.

I see the Background as a refactoring opportunity, e.g. extract common setup steps so we have the “situation” common to the feature. However, in some scenarios within the same feature it’s not very practical since we use the checkpoints, and often this has to do with networking vs no networking. In ssh.feature we need networking in every scenario, but in e.g. torified_browsing.feature we don’t, so there’s no Background. I think there’s some opportunity to split more features like we intend in Feature #5707, and then actually re-introduce Backgrounds (e.g. from torified_browsering.feature we could split out scenarios relating to configuration and not torified usage).

> There’s trailing whitespace in features/step_definitions/snapshots.rb.

commit:616d952

> In step_regex = Regexp.new(…), the arbitrary text that’s turned into a regexp should see its regexp special chars escaped, no?

Well, that makes it impossible to put regex special chars into the :descriptions, should we need it (really, though, I don’t see why we ever would).

Actually, I think it’s be better to drop the :description field and instead put a :step_regex that already is a Regexp, much how we define normal steps and actually are used to. E.g.:

    'tails-greeter' => {
      :step_regex => /^Tails has booted from DVD without network and stopped at Tails Greeter's login screen$/,

What do you think?

> I’ve not finished my review but I didn’t see if/where the removed “Then Pidgin has the expected accounts configured with random nicknames” has been re-added.

Good catch! It was indeed lost. Re-introduced in commit:cc63860,

> While reading save_snapshot it’s easy to believe that when internal_snapshot = false we’re doing the opposite of “internal” snapshots, that is “external” snapshots. Apparently this is not the case. A comment, or improving variable naming, would fix that. Note that later on in the same file I see the nomenclature is “ram_only_snapshot” vs. “internal_snapshot”. Reuse it consistently?

commit:858baa2

> I’m not convinced by the move of stuff to features/time_syncing_bridges.feature. Can we merge it back into time_syncing.feature? If needed, it might be that one can define a Background in the middle of a feature and see it applied only to the scenarios that follow.

Cucumber doesn’t support multiple Backgrounds in the same feature, but I agree that they should be merged (the Backgrounds would only be one step, so it’s no big deal). Any way, I merged them in commit:2eb0238.

I also noticed that “Scenario: Clock way in the past” was removed. I think it was removed based on bad reasoning (commit:394fcf5). We still need a scenario that test the situation were Tails release date was a while ago. Hence I suggest commit:b396ca5.

> Probably off-topic, but it looks like the time syncing and Tor bridges tests would be good candidates for DRY’ing feature files a bit with scenario outlines. Want a ticket about it as part of the test suite refactoring / cleanup project?

Oh, yes. I created Feature #10328.

> There’s at least one occurence of “two Given steps in a row” caused by semi-mechanical adjustements.

commit:3f407fe + Feature #10329

> “Scenario: Copying video files to a persistence and making sure that they persist”’s name seems to not match what it currently does.

commit:658490c

> The simple “Scenario: Installing Tails to a pristine USB drive” has apparently disappeared, and b151827 doesn’t really explain why. I guess that’s because this work is done somewhere else in a hidden way. If this guess is right, then I don’t like it: that scenario describes one of the most common use cases of Tails Installer, and one purpose of Gherkin features is to document how stuff should work, so IMO this specific behaviour is totally worth being explicitly documented in Gherkin. Similarly, it looks like “Scenario: Booting Tails from a USB drive without a persistent partition and creating one” has disappeared and IMO at least some part of it should come back somehow.

I see your point and I actually considered this, although admittedly in a poor way without much explanation. Case in point: the optimizations we do given the new snapshot system somewhat works against Gherkin expressibility. It’s clear that we cannot have both full Gherkin expressibility and a fast test suite at the same time, so we need to find the sweet spot that works for us, some kind of compromise.

If we re-add those scenarios (in some form), they will just duplicate stuff we already do behind the scenes, like you observed. Furthermore, the result from these scenarios is not what subsequent scenarios will test, but that shouldn’t be a problem. I optimistically pushed something as commit:f656715. What do you think?

(Note that the reference so ‘USB drive “current”’ is pretty ugly in the second scenario. That step should be changed to “the boot drive” or something. I intend to fix all that once Feature #5847 is fixed. I’ve added a comment there.)

> In “Scenario: Booting Tails from a USB drive in UEFI mode” the Given/Then/Given/When sequence feels strange.

Feature #10329

> I see “XXX: The next step succeeds […]” and a few “XXX: how does guestfs work vs snapshots?” => please create tickets if you think they can be useful, or something.

Bug #10330

> Typo in “including ones makred”.

commit:a33674e

> And that’s all for this first full review of the diff :)

Thanks a lot for the reviews, both of you, intrigeri and kytv! :)

Also note that I pushed a few more useful commits so => review but before I actually put it back for review I’ll want your opinions on at least the two questions I have above.

#54 Updated by anonym 2015-10-04 02:55:49

#55 Updated by anonym 2015-10-04 02:56:42

  • Priority changed from Normal to High

Raising priority since we need this merged in about a week.

#56 Updated by intrigeri 2015-10-05 04:53:52

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

(Here too I’m skipping most of the bits I’m satisfied with — congrats for those!)

>> Perhaps some “how to write tests” doc update would be useful? E.g. that doc still
>> mentions skip_steps_while_restoring_background, which feels wrong.

> commit:381d61a

Yay.

  • “Snapshots may have parent’s” <- maybe “parents” instead?
  • “can be maked” <- maybe “made” instead?
  • “If set to `true`, the snapshot” <- what is set to true?

>> “Tails has booted from DVD and logged in with” <= I think there’s a grammar bug in here.

> Care to elaborate? I don’t quite get it.

“Tails” is not a subject that can log in. A user does log in.

> “and logged in” => “is logged in”? It definitely isn’t the most perfect sentence construction; suggestions for improvements are welcome!

E.g.:

  • “I am logged into a Tails started from DVD”
  • “I have started Tails from DVD and logged in”

>> LIBVIRT_DOMAIN_NAME, LIBVIRT_DOMAIN_UUID etc. look more like internal/private constants (suitable for features/support/env.rb maybe), than anything we should expose in a public configuration interface, no?

> I originally made them configurable like this to test how running multiple instances of the test suite under the same host would work out. Spoiler: there’s some more work left to do that! :)

> Any way, I personally found this feature quite useful; when developing this branch I used it to re-configured LIBVIRT_{DOMAIN,NETWORK}_NAME differently to have less issues when switching to/from this branch, so, as a test suite developer, I quite like having this flexibility. It will be useful in future branches that break VM compatibility with the base branches, or similar.

> Of course, changing them in such a way as I needed is only one commit away if we make them into constants. Whatever. If you feel like they should go, I’ll make them go.

> So what do you think?

I now see your point, thanks for explaining the usefulness of the current implementation. I’m not convinced that it’s the right way to implement “running multiple instances of the test suite under the same host”, which seems to call for the test suite automatically defining unique values for these variables, instead of making them configurable, so I’m in favour of not adding them to the public config interface and coming back to it if/when we need/want to implement “running multiple instances of the test suite under the same host” some day.

>> A few newly introduced features have “As a” and “I want” but miss the “In order to” […]

> Right. Let’s focus on this in another ticket, since it’s basically fucked in every feature. I filed Feature #10329 for this.

Fair enough. Thanks for filing that other ticket.

>> In “Scenario: Using a persistent Electrum configuration”, I’m not sure why we use a checkpoint that has networking disabled, and then plug it in. Same in “Scenario: Using a persistent Pidgin configuration”.

> There is no checkpoint with persistence + networking. It seemed to me that the most common case for persistence is without networking, and timewise it’s pretty cheap (we will have to wait for Tor to working in either case). Adding a peristence + networking checkpoint it trivial, but will cost us ~900 MiB disk space in TMPDIR. If you think it’s worth it vs the isotester specs, I’ll go ahead an add it.

This would require benchmarking of a full test suite run (so that disk cache is taken into account), and benchmarking would require the test suite to run reliably on lizard, which is not the case yet => let’s ignore this for now.

>> In step_regex = Regexp.new(…), the arbitrary text that’s turned into a regexp should see its regexp special chars escaped, no?

> Well, that makes it impossible to put regex special chars into the :descriptions, should we need it (really, though, I don’t see why we ever would).

> Actually, I think it’s be better to drop the :description field and instead put a :step_regex that already is a Regexp, much how we define normal steps and actually are used to. E.g.: […] What do you think?

Yes, please :)

> I also noticed that “Scenario: Clock way in the past” was removed. I think it was removed based on bad reasoning (commit:394fcf5).

Indeed.

> We still need a scenario that test the situation were Tails release date was a while ago. Hence I suggest commit:b396ca5.

I’d like to be nice to users who only boot Tails every 3 months or so => perhaps s/6 weeks/13 weeks/ or something?

>> The simple “Scenario: Installing Tails to a pristine USB drive” has apparently disappeared, and b151827 doesn’t really explain why. I guess that’s because this work is done somewhere else in a hidden way. If this guess is right, then I don’t like it: that scenario describes one of the most common use cases of Tails Installer, and one purpose of Gherkin features is to document how stuff should work, so IMO this specific behaviour is totally worth being explicitly documented in Gherkin. Similarly, it looks like “Scenario: Booting Tails from a USB drive without a persistent partition and creating one” has disappeared and IMO at least some part of it should come back somehow.

> I see your point and I actually considered this, although admittedly in a poor way without much explanation. Case in point: the optimizations we do given the new snapshot system somewhat works against Gherkin expressibility. It’s clear that we cannot have both full Gherkin expressibility and a fast test suite at the same time, so we need to find the sweet spot that works for us, some kind of compromise.

Got it. Fundamentally, we would need a way to turn a Gherkin scenario into a checkpoint. If we could pass parameters to tags (can we?) then we could attach the metadata we need (parent checkpoint, and name of the resulting checkpoint) to a scenario that describes a checkpoint. Or whatever other way to have this. If that doesn’t sound like a crazy idea, file a ticket about it for later? And please don’t spend too much time on it now, it’s not a blocker and our plate is quite full this month ;)

> If we re-add those scenarios (in some form), they will just duplicate stuff we already do behind the scenes, like you observed. Furthermore, the result from these scenarios is not what subsequent scenarios will test, but that shouldn’t be a problem. I optimistically pushed something as commit:f656715. What do you think?

OK, that’ll do for now.

> (Note that the reference so ‘USB drive “current”’ is pretty ugly in the second scenario. That step should be changed to “the boot drive” or something. I intend to fix all that once Feature #5847 is fixed. I’ve added a comment there.)

Cool.

> Also note that I pushed a few more useful commits so => review but before I actually put it back for review I’ll want your opinions on at least the two questions I have above.

Reviewed up to commit:f544072 (that includes a commit I did myself last week and forgot to push), looks good so far!

There’s a typo in “global varaibles” (M-x flyspell-mode, anyone? ;)

#57 Updated by anonym 2015-10-06 05:09:56

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

intrigeri wrote:
> >> Perhaps some “how to write tests” doc update would be useful? E.g. that doc still
> >> mentions skip_steps_while_restoring_background, which feels wrong.
>
> > commit:381d61a
>
> Yay.
>
> * “Snapshots may have parent’s” <- maybe “parents” instead?
> * “can be maked” <- maybe “made” instead?

commit:3a28e38

> * “If set to `true`, the snapshot” <- what is set to true?

commit:b1e0444

> >> “Tails has booted from DVD and logged in with” <= I think there’s a grammar bug in here.
>
> > Care to elaborate? I don’t quite get it.
>
> “Tails” is not a subject that can log in. A user does log in.
>
> > “and logged in” => “is logged in”? It definitely isn’t the most perfect sentence construction; suggestions for improvements are welcome!
>
> E.g.:
>
> * “I am logged into a Tails started from DVD”
> * “I have started Tails from DVD and logged in”

Indeed, that l. Let’s move from the 'Tails has booted Tails' construction to 'I have started Tails', with the “have” part being what signifies that a snapshot was used. Thanks for the suggestion! Fixed in commit:83bde0f.

> >> LIBVIRT_DOMAIN_NAME, LIBVIRT_DOMAIN_UUID etc. look more like internal/private constants (suitable for features/support/env.rb maybe), than anything we should expose in a public configuration interface, no?
>
> > I originally made them configurable like this to test how running multiple instances of the test suite under the same host would work out. Spoiler: there’s some more work left to do that! :)
>
> > Any way, I personally found this feature quite useful; when developing this branch I used it to re-configured LIBVIRT_{DOMAIN,NETWORK}_NAME differently to have less issues when switching to/from this branch, so, as a test suite developer, I quite like having this flexibility. It will be useful in future branches that break VM compatibility with the base branches, or similar.
>
> > Of course, changing them in such a way as I needed is only one commit away if we make them into constants. Whatever. If you feel like they should go, I’ll make them go.
>
> > So what do you think?
>
> I now see your point, thanks for explaining the usefulness of the current implementation. I’m not convinced that it’s the right way to implement “running multiple instances of the test suite under the same host”, which seems to call for the test suite automatically defining unique values for these variables, instead of making them configurable, so I’m in favour of not adding them to the public config interface and coming back to it if/when we need/want to implement “running multiple instances of the test suite under the same host” some day.

Ok. In order to support --keep-snapshots we’d need a way to make such “generated” (or whatever) names/uuids persistent for each test suite instance (whatever decides that — in what I did it would be the local configuration). Blah. This is not relevant. I made them into constants in commit:b7397ec.

> >> In step_regex = Regexp.new(…), the arbitrary text that’s turned into a regexp should see its regexp special chars escaped, no?
>
> > Well, that makes it impossible to put regex special chars into the :descriptions, should we need it (really, though, I don’t see why we ever would).
>
> > Actually, I think it’s be better to drop the :description field and instead put a :step_regex that already is a Regexp, much how we define normal steps and actually are used to. E.g.: […] What do you think?
>
> Yes, please :)

Actually, this won’t make sense. We treat the :description as the matched and ready instance of the “step name”, since that’s what we print via the debug formatter later. I instead did what you said, i.e. add regex escaping, since it does make sense with that in mind. commit:4af2f7a.

> > We still need a scenario that test the situation were Tails release date was a while ago. Hence I suggest commit:b396ca5.
>
> I’d like to be nice to users who only boot Tails every 3 months or so => perhaps s/6 weeks/13 weeks/ or something?

commit:b301ef9

> >> The simple “Scenario: Installing Tails to a pristine USB drive” has apparently disappeared, and b151827 doesn’t really explain why. I guess that’s because this work is done somewhere else in a hidden way. If this guess is right, then I don’t like it: that scenario describes one of the most common use cases of Tails Installer, and one purpose of Gherkin features is to document how stuff should work, so IMO this specific behaviour is totally worth being explicitly documented in Gherkin. Similarly, it looks like “Scenario: Booting Tails from a USB drive without a persistent partition and creating one” has disappeared and IMO at least some part of it should come back somehow.
>
> > I see your point and I actually considered this, although admittedly in a poor way without much explanation. Case in point: the optimizations we do given the new snapshot system somewhat works against Gherkin expressibility. It’s clear that we cannot have both full Gherkin expressibility and a fast test suite at the same time, so we need to find the sweet spot that works for us, some kind of compromise.
>
> Got it. Fundamentally, we would need a way to turn a Gherkin scenario into a checkpoint.

Exactly!

> If we could pass parameters to tags (can we?)

Nope.

> then we could attach the metadata we need (parent checkpoint, and name of the resulting checkpoint) to a scenario that describes a checkpoint.

Well, the “parent” metadata can be decided by the first step, replacing :parent_checkpoint. A @temporary_checkpoint tag could replace :temporary. Hmm.

> Or whatever other way to have this. If that doesn’t sound like a crazy idea, file a ticket about it for later? And please don’t spend too much time on it now, it’s not a blocker and our plate is quite full this month ;)

Filed Feature #10341. Seems interesting after spending only five minutes on this (so I could have missed some fatal issue with this approach).

> > Also note that I pushed a few more useful commits so => review but before I actually put it back for review I’ll want your opinions on at least the two questions I have above.
>
> Reviewed up to commit:f544072 (that includes a commit I did myself last week and forgot to push), looks good so far!

Yay! Your commit:02b5dcb indeed makes sense (consider it “reviewed”).

> There’s a typo in “global varaibles”

commit:3a28e38

> (M-x flyspell-mode, anyone? ;)

I’ve now made this mode always on. :)

#58 Updated by intrigeri 2015-10-12 10:23:21

Yay for all the fixes and improvements!

Building a current ISO, will run tests, and then merge unless I see regressions. Woohoo :)

#59 Updated by intrigeri 2015-10-12 11:47:10

  • % Done changed from 50 to 70

#60 Updated by intrigeri 2015-10-12 14:17:23

While testing this branch, with an ISO built from current devel, I see “Scenario: Install packages using apt-get” fail to do its post-background-restore:

  @check_tor_leaks
  Scenario: Install packages using apt-get                                                                         # features/apt.feature:15
calling as root: echo 'hello?'
call returned: [0, "hello?\n", ""]
[log] CLICK on (1024,384)
calling as root: /sbin/ifconfig eth0 | grep -q 'inet addr'
call returned: [0, "", ""]
calling as root: service tor status
call returned: [0, "tor is running.\n", ""]
calling as root: service tor stop
call returned: [0, "Stopping tor daemon...done.\n", ""]
calling as root: rm -f /var/log/tor/log
call returned: [0, "", ""]
calling as root: date -s '@1444683265'
call returned: [0, "Mon Oct 12 20:54:25 UTC 2015\n", ""]
calling as root: restart-tor

… and 10 minutes later I still see no call returned. I’ve seen Vidalia being killed, reappearing, and its icon becoming green, so in my book Tor has bootstrapped and restart-tor should normally have exited.

I can’t reproduce this with a devel test suite against an ISO built from devel.

#61 Updated by intrigeri 2015-10-12 14:54:07

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

Reproduced on a different system (sid, the initial report was on Jessie), so I need help. anonym, any idea? Any more info I should provide?

#62 Updated by anonym 2015-10-13 07:34:42

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

intrigeri wrote:

> calling as root: restart-tor

That is wrong. In merge commit:6c22d3f I did an incorrect conflict resolution, now corrected in commit commit:61ed522. (Disclaimer: the merge conflict was resolved correctly in experimental, and that’s what I’ve mostly been running lately, as an integration branch for all this test suite craziness. Sorry!)

That said, it’s weird that execute('restart-tor') "hangs". It could be that there's some bug in restart-tor@ that prevents it from exiting. I actually think there may be an issue that I didn’t think about when merging that branch, but I’ll post that on Feature #9516. Switching to spawn should work around it by making wait_until_tor_is_working fail, instead of freezing.

#63 Updated by intrigeri 2015-10-14 00:01:30

  • blocks Feature #10287: Set up limited email notification on automatic test failure for the initial deployment added

#64 Updated by intrigeri 2015-10-14 00:05:02

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

Applied in changeset commit:a804982ae08a693e8eed96e477ac07f47c24de96.

#65 Updated by intrigeri 2015-10-14 00:05:53

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

Yeepee :)

#66 Updated by bertagaz 2015-10-14 02:31:18

\o/

Feature #6094 FTW!!!

#67 Updated by bertagaz 2015-10-19 03:45:49

  • blocked by deleted (Feature #10287: Set up limited email notification on automatic test failure for the initial deployment)

#68 Updated by anonym 2015-11-03 11:31:49

  • Status changed from Fix committed to Resolved

#69 Updated by intrigeri 2016-02-21 21:46:09

(Top secret CI team board)

kytv! The 6094 crew wants you back, we miss you <3