Project

General

Profile

Feature #10148

Split test suite capture videos per-scenario

Added by intrigeri over 4 years ago. Updated over 4 years ago.

Status:
Resolved
Priority:
Normal
Assignee:
-
Category:
Test suite
Target version:
Start date:
09/02/2015
Due date:
% Done:

100%

Feature Branch:
test/10148-capture-per-scenario-videos
Type of work:
Code
Blueprint:
Starter:
Affected tool:

Description

This would make such videos much easier to use for debugging, and would also allow us to archive video only for failing scenario on Jenkins.


Related issues

Related to Tails - Feature #8947: Use the headless Ruby gem in the automated test suite Rejected 02/24/2015
Related to Tails - Feature #10151: Organize automated test suite artifacts in per-run directories Resolved 09/02/2015
Blocks Tails - Bug #10001: Consider using less CPU-hungry codec for the test suite's --capture option Resolved 08/14/2015
Blocks Tails - Feature #10154: Store automated test videos with other artifacts Resolved 09/03/2015

Associated revisions

Revision 884e4834 (diff)
Added by anonym over 4 years ago

Split --capture videos per scenario.

Will-fix: #10148

Revision 1a749b0a
Added by intrigeri over 4 years ago

Merge remote-tracking branch 'origin/test/10148-capture-per-scenario-videos' into stable

Fix-committed: #10148, #10001

History

#1 Updated by intrigeri over 4 years ago

  • Related to Feature #8947: Use the headless Ruby gem in the automated test suite added

#2 Updated by anonym over 4 years ago

  • Assignee set to anonym

#3 Updated by anonym over 4 years ago

  • Status changed from Confirmed to In Progress

#4 Updated by anonym over 4 years ago

  • Assignee deleted (anonym)
  • Target version set to Tails_1.6
  • % Done changed from 0 to 50
  • QA Check set to Ready for QA
  • Feature Branch set to test/10148-capture-per-scenario-videos

#5 Updated by intrigeri over 4 years ago

  • Assignee set to kytv

#6 Updated by kytv over 4 years ago

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

Seems to work well except when --capture isn't used.


@product
Feature: Encryption and verification using GnuPG
  As a Tails user
  I want to be able to easily encrypt and sign messages using GnuPG
  And decrypt and verify GnuPG blocks

  Background:                                                            # features/encryption.feature:7
    Given a computer                                                     # features/step_definitions/common_steps.rb:77
    And I start Tails from DVD with network unplugged and I login        # features/step_definitions/common_steps.rb:165
    And I generate an OpenPGP key named "test" with password "asdf"      # features/step_definitions/encryption.rb:11
    And I save the state so the background can be restored next scenario # features/step_definitions/common_steps.rb:418

  Scenario: Encryption and decryption using Tails OpenPGP Applet         # features/encryption.feature:13
    When I type a message into gedit                                     # features/step_definitions/encryption.rb:34
    And I encrypt the message using my OpenPGP key                       # features/step_definitions/encryption.rb:82
    Then I can decrypt the encrypted message                             # features/step_definitions/encryption.rb:89
      no implicit conversion of nil into String (TypeError)
      /home/kytv/tails/features/support/hooks.rb:151:in `exist?'
      /home/kytv/tails/features/support/hooks.rb:151:in `After'

  Scenario: Signing and verification using Tails OpenPGP Applet          # features/encryption.feature:18
    Given a computer                                                     # features/step_definitions/common_steps.rb:77
      undefined method `pid' for nil:NilClass (NoMethodError)
      ./features/support/helpers/display_helper.rb:34:in `stop'
      ./features/support/helpers/vm_helper.rb:84:in `destroy_and_undefine'
      ./features/support/helpers/vm_helper.rb:70:in `rescue in initialize'
      ./features/support/helpers/vm_helper.rb:60:in `initialize'
      ./features/step_definitions/common_steps.rb:79:in `new'
      ./features/step_definitions/common_steps.rb:79:in `/^a computer$/'
      features/encryption.feature:8:in `Given a computer'
    When I type a message into gedit                                     # features/step_definitions/encryption.rb:34
    And I sign the message using my OpenPGP key                          # features/step_definitions/encryption.rb:95
    Then I can verify the message's signature                            # features/step_definitions/encryption.rb:102
Scenario failed at time 00:04:05
Took screenshot "/tmp/TailsToaster/encryption-2015-09-04T21:18:41+00:00.png" 

  Scenario: Encryption/signing and decryption/verification using Tails OpenPGP Applet # features/encryption.feature:23
    When I type a message into gedit                                                  # features/step_definitions/encryption.rb:34
    And I both encrypt and sign the message using my OpenPGP key                      # features/step_definitions/encryption.rb:108
    Then I can decrypt and verify the encrypted message                               # features/step_definitions/encryption.rb:116
Scenario failed at time 00:04:05
Took screenshot "/tmp/TailsToaster/encryption-2015-09-04T21:18:42+00:00.png" 

  Scenario: Symmetric encryption and decryption using Tails OpenPGP Applet # features/encryption.feature:28
    When I type a message into gedit                                       # features/step_definitions/encryption.rb:34
    And I symmetrically encrypt the message with password "asdf"           # features/step_definitions/encryption.rb:123
    Then I can decrypt the encrypted message                               # features/step_definitions/encryption.rb:89
Scenario failed at time 00:04:06
Took screenshot "/tmp/TailsToaster/encryption-2015-09-04T21:18:42+00:00.png" 

#7 Updated by kytv over 4 years ago

  • Blocks Bug #10001: Consider using less CPU-hungry codec for the test suite's --capture option added

#8 Updated by kytv over 4 years ago

It looks like this simple change will fix it.

diff --git a/features/support/hooks.rb b/features/support/hooks.rb
index c909471..ce7ccde 100644
--- a/features/support/hooks.rb
+++ b/features/support/hooks.rb
@@ -148,7 +148,7 @@ After('@product') do |scenario|
       STDIN.gets
     end
   else
-    if File.exist?(@video_path) && not($config['CAPTURE_ALL'])
+    if @video_path && File.exist?(@video_path) && not($config['CAPTURE_ALL'])
       FileUtils.rm(@video_path)
     end
   end

#9 Updated by anonym over 4 years ago

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

kytv wrote:

It looks like this simple change will fix it.

[...]

Hah, you are of course correct. Great, apparently I didn't test without the --capture feature. Patch applied!

#10 Updated by kytv over 4 years ago

  • Assignee changed from kytv to intrigeri

Now I'm happy with this branch. :)

#11 Updated by intrigeri over 4 years ago

  • Assignee changed from intrigeri to anonym
  • QA Check changed from Ready for QA to Dev Needed
    bad_unix_filename_chars = Regexp.new("[\s/]")
    video_name.gsub!(bad_unix_filename_chars, '_')

What's this supposed to do? I suspect I'd much rather see a whitelist approach here.

CAPTURE and CAPTURE_ALL need to be documented in wiki/src/contribute/release_process/test/usage.mdwn.

Otherwise, looks good!

#12 Updated by intrigeri over 4 years ago

Also, I see:

./run_test_suite: line 93: --: command not found
./run_test_suite: line 93: --format: command not found
./run_test_suite: line 93: --format: command not found
./run_test_suite: line 94: --out: command not found

... and --help is buggy (the explanation is quite obvious when one looks at the code).

#13 Updated by anonym over 4 years ago

intrigeri wrote:

[...]

What's this supposed to do?

Remove whitespaces and forward slashes, which you probably already have suspected.

I suspect I'd much rather see a whitelist approach here.

Ok, fix pushed.

CAPTURE and CAPTURE_ALL need to be documented in wiki/src/contribute/release_process/test/usage.mdwn.

Otherwise, looks good!

Done.

#14 Updated by anonym over 4 years ago

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

intrigeri wrote:

Also, I see:

[...]

... and --help is buggy (the explanation is quite obvious when one looks at the code).

Lol, fixed!

#15 Updated by intrigeri over 4 years ago

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

Thanks for the fixes :)

Can we add : to the filename chars whitelist? The timestamps look ugly without it. Also, adding + would make the timezone info a bit more useful (unless we simply want to convert to UTC?).

I wonder if we should perhaps display the full path to the video capture upon failure, much like we've been doing forever for screenshots. This would save some time when debugging (in particular when --capture-all is used, but also when the temp directory is filled with videos from previous runs). What do you think? Assuming it's a good idea: is it trivial enough to be dealt with as part of this ticket, or do you want another one?

I also have doubts wrt. the video files naming:

  • I'm not sure if capture- prefix adds any value, given we have the filename extension already and we don't put any other videos in that directory AFAIK.
  • Perhaps make it more similar to the screenshots we save? Those files have very similar purposes, and are named very differently with this branch. Maybe we should simply merge both filename formats, to include scenario.feature.file + some separator + scenario.name (and sanitize the result) in all cases?
  • Video filename embeds TIME_AT_START, while screenshots filename embeds the failure time. It feels a bit confusing. Renaming the video upon failure would be inconsistent as well (wrt. --capture-all this time). Perhaps start time could be used for screenshots as well? This one I'm less sure, feel free to disregard if you don't think it's important.

Slightly OT: if we ever need to save a few more percents of test suite runtime, we could delay the video compression to only do it in case we'll keep it in the end (failed scenario under --capture and no --capture-all). Not sure the impact will be measurable at all in practice, though.

#16 Updated by anonym over 4 years ago

intrigeri wrote:

Thanks for the fixes :)

Can we add : to the filename chars whitelist? The timestamps look ugly without it. Also, adding + would make the timezone info a bit more useful (unless we simply want to convert to UTC?).

Certainly, good ideas all of them. Fixed.

I wonder if we should perhaps display the full path to the video capture upon failure, much like we've been doing forever for screenshots. This would save some time when debugging (in particular when --capture-all is used, but also when the temp directory is filled with videos from previous runs). What do you think? Assuming it's a good idea: is it trivial enough to be dealt with as part of this ticket, or do you want another one?

Definitely. Have a look at #10151's feature branch, which already does that, and fixes this output to be more ordered and consistent. I think your remaining concerns could be fixed in that branch too, and be ignore for this ticket. If you agree, let's move this discussion there.

I also have doubts wrt. the video files naming:

  • I'm not sure if capture- prefix adds any value, given we have the filename extension already and we don't put any other videos in that directory AFAIK.

Ok, this makes sense. Same for the screenshot and .pcap files then.

  • Perhaps make it more similar to the screenshots we save? Those files have very similar purposes, and are named very differently with this branch. Maybe we should simply merge both filename formats, to include scenario.feature.file + some separator + scenario.name (and sanitize the result) in all cases?

Sure. We will get very long filenames, but whatever.

  • Video filename embeds TIME_AT_START, while screenshots filename embeds the failure time. It feels a bit confusing. Renaming the video upon failure would be inconsistent as well (wrt. --capture-all this time). Perhaps start time could be used for screenshots as well? This one I'm less sure, feel free to disregard if you don't think it's important.

Again, this is done in the sanest way I can think of in #10151.

Slightly OT: if we ever need to save a few more percents of test suite runtime, we could delay the video compression to only do it in case we'll keep it in the end (failed scenario under --capture and no --capture-all). Not sure the impact will be measurable at all in practice, though.

I'm not sure this will save much. When it's done on the fly, in parallel with the test run, it doesn't impact it so much. Blocking progress in the end of a failed scenario to re-encode the video would possibly worse.

#17 Updated by intrigeri over 4 years ago

Definitely. Have a look at #10151's feature branch, which already does that, and fixes this output to be more ordered and consistent. I think your remaining concerns could be fixed in that branch too, and be ignore for this ticket. If you agree, let's move this discussion there.

Fine with me => please sum up there whatever I raised and is not covered by that branch yet, and then assign back to me for QA once you think this branch is ready (which seems to be the case already, but your call).

#18 Updated by anonym over 4 years ago

  • Related to Feature #10151: Organize automated test suite artifacts in per-run directories added

#19 Updated by anonym over 4 years ago

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

intrigeri wrote:

Definitely. Have a look at #10151's feature branch, which already does that, and fixes this output to be more ordered and consistent. I think your remaining concerns could be fixed in that branch too, and be ignore for this ticket. If you agree, let's move this discussion there.

Fine with me => please sum up there whatever I raised and is not covered by that branch yet, and then assign back to me for QA once you think this branch is ready (which seems to be the case already, but your call).

Done (well, I quoted everything, but that's all I'll need post-1.6).

#20 Updated by intrigeri over 4 years ago

  • Blocks Feature #10154: Store automated test videos with other artifacts added

#21 Updated by intrigeri over 4 years ago

  • Status changed from In Progress to 11
  • % Done changed from 50 to 100

#22 Updated by intrigeri over 4 years ago

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

Excellent :)

#23 Updated by bertagaz over 4 years ago

  • Status changed from 11 to Resolved
  • QA Check deleted (Pass)

Also available in: Atom PDF