Project

General

Profile

Bug #8907

Bug #8059: Windows Camouflage automated tests sometimes failed due to differently ordered icons in the notification area

storage_helper.rb does not check available disk space

Added by kytv almost 5 years ago. Updated almost 5 years ago.

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

100%

Feature Branch:
kytv:test/8907-check-space
Type of work:
Code
Blueprint:
Starter:
Affected tool:

Description

The test suite will currently try to create storage devices without checking whether the required disk space is available. Of course, test(s) will eventually fail but it would be preferable to fail much earlier in the process.

I'm currently testing a patch that addresses this.

Associated revisions

Revision 4920317a
Added by Tails developers almost 5 years ago

Merge remote-tracking branch 'kytv/test/8907-check-space' into testing

Fix-committed: #8907

History

#1 Updated by kytv almost 5 years ago

  • Subject changed from storage_helper.rb does not check available disk space before working to storage_helper.rb does not check available disk space

#2 Updated by kytv almost 5 years ago

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

#3 Updated by kytv almost 5 years ago

  • Assignee changed from kytv to anonym
  • % Done changed from 20 to 50
  • QA Check set to Ready for QA
  • Feature Branch set to kytv:test/8907-check-space

#4 Updated by anonym almost 5 years ago

The branch works fine, but here's some nitpicking: :)

+ free = `df "#{path}"`

In features/support/helpers/misc_helpers.rb we define cmd_helper(), which asserts that the command succeeds. In this case you check that path exists, which should catch all but truly arcane errors (like bit flips), but just like we should prefer execute_successfully over an un-checked execute, we should consistently use cmd_helper(), imho.

+ avail = convert_to_MiB(get_free_space('host', "#{@pool_path}"), "KiB")

The embed-expression-inside-string mechanism #{...} isn't needed here, and just makes the code harder to read.

+ needed = convert_to_MiB(options[:size].to_i, options[:unit]) + 500

Where does this magic number 500 come from? I'm know there's a good explanation so I think you should introduce an aptly named variable + a comment.

+ assert(avail > needed, "Error creating disk \"#{name}\" in \"#{@pool_path}\". Need " +

Strictly (and semantically) speaking, we should assert that avail >= needed. :)

#5 Updated by kytv almost 5 years ago

  • % Done changed from 50 to 60

anonym wrote:

The branch works fine, but here's some nitpicking: :)

Thank you for your continued guidance and explanations.

The magical number's value is arbitrary. I just wanted some sort of padding or slack space and thought 1GB might be too much but that around 500MB might be enough.

Perhaps the added extrapadding variable and explanation is sufficient. Changes made, tested, pushed.

#6 Updated by anonym almost 5 years ago

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

kytv wrote:

The magical number's value is arbitrary. I just wanted some sort of padding or slack space and thought 1GB might be too much but that around 500MB might be enough.

Makes sense.

Perhaps the added extrapadding variable and explanation is sufficient. Changes made, tested, pushed.

I think I'd prefer it like this:

--- a/features/support/helpers/storage_helper.rb
+++ b/features/support/helpers/storage_helper.rb
@@ -72,12 +72,13 @@ class VMStorage
     options[:type] ||= "qcow2" 
     # Require 'slightly' more space to be available to give a bit more leeway
     # with rounding, temp file creation, etc.
-    extrapadding = 500
-    needed = convert_to_MiB(options[:size].to_i, options[:unit]) + extrapadding
+    reserved = 500
+    needed = convert_to_MiB(options[:size].to_i, options[:unit])
     avail = convert_to_MiB(get_free_space('host', @pool_path), "KiB")
-    assert(avail >= needed, "Error creating disk \"#{name}\" in \"#{@pool_path}\"." +
-                            "Need #{needed} MiB but only #{avail} MiB is available.")
-
+    assert(avail - reserved >= needed,
+           "Error creating disk \"#{name}\" in \"#{@pool_path}\". " \
+           "Need #{needed} MiB but only #{avail} MiB is available of, " \
+           "which #{reserved} MiB is reserved for other temporary files.")
     begin
       old_vol = @pool.lookup_volume_by_name(name)
     rescue Libvirt::RetrieveError

This way it's less likely to confuse users about where those 500 MiB of free space went. :) What do you think?

#7 Updated by kytv almost 5 years ago

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

Yes, that's much better. Applied and pushed.

#8 Updated by Tails almost 5 years ago

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

Applied in changeset commit:1eec6e8a0cd07c9d43b76e3dcf972e37e3a276ce.

#9 Updated by anonym almost 5 years ago

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

#10 Updated by BitingBird almost 5 years ago

  • Status changed from 11 to Resolved

#11 Updated by intrigeri almost 5 years ago

  • Parent task set to #8539

Also available in: Atom PDF