Tails

#1 Updated by anonym 2015-04-29 03:20:28

• Status changed from Confirmed to In Progress

Applied in changeset commit:2e4c214c556f8b298d7e06011b06e1e069327d86.

#2 Updated by anonym 2015-04-29 03:20:55

• Tracker changed from Bug to Feature
• Assignee deleted (anonym)
• % Done changed from 0 to 50
• QA Check set to Ready for QA
• Feature Branch set to test/9290-nested-try_for

#3 Updated by intrigeri 2015-04-29 03:26:24

• Assignee set to kytv

#4 Updated by anonym 2015-04-29 04:02:40

• blocked by Bug #9189: The remote shell still hangs sometimes on Jessie hosts added

#5 Updated by anonym 2015-04-29 04:03:42

• Assignee deleted (kytv)

Note that this branch is based on test/9189-allow-timeouts-in-try_for from Bug #9189, hence the blocking relationship.

#6 Updated by anonym 2015-04-29 04:03:59

• Assignee set to kytv

#8 Updated by kytv 2015-05-01 12:29:32

• Assignee changed from kytv to intrigeri

Testing also passed.

#9 Updated by intrigeri 2015-05-01 13:27:38

• Assignee changed from intrigeri to anonym
• % Done changed from 50 to 60
• QA Check changed from Ready for QA to Info Needed

The code doesn’t seem scary, and I could almost understand the proof enough to be convinced. Also, I totally agree that it’s a good thing to generalize the try_for function to avoid future headaches. However, we have no use case for it in the test suite yet, that could serve as a testbed to help convince me that indeed, it does work fine when nested. Hence two questions:

• did you try nesting this new try_for, in a way that confirms that indeed the unique exceptions are caught and re-thrown in the right place?
• could you try to add an actual use case for nested try_for in our current test suite, so that we can indeed see that it works fine in practice?

If you’ve tested it already, I’m happy to take your word for it, and then feel free to merge this branch yourself. Also, I don’t mean to make you spend too much time on this, and perhaps anyway it’s worth postponing to 1.4.1, given how your plate for 1.4 is pretty much full already.

#10 Updated by anonym 2015-05-02 03:20:46

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

intrigeri wrote:
> * did you try nesting this new try_for, in a way that confirms that indeed the unique exceptions are caught and re-thrown in the right place?

Yes. I tested it with this code (that I still had in my shell history):

t = Time.now
try_for(23, :msg => "level 1: fail", :delay => 0) do
  STDERR.puts "level 1: start #{Time.new - t}"
  try_for(10, :msg => "level 2: fail", :delay => 0) do
    STDERR.puts "level 2: start #{Time.new - t}"
    try_for(4, :msg => "level 3: fail", :delay => 0) do
      STDERR.puts "level 3: start #{Time.new - t}"
      sleep 1000
    end
  end
end
STDERR.puts "finished: #{Time.new - t}"

and then verified that:

• only the “level 1” exception is raised, and only at the end
• the switching between blocks makes sense (i.e. the “level X: start” order that’s printed makes sense)
• the timeouts happen as expected (also note that I picked the three timings so no combination can add up to any other one)

> * could you try to add an actual use case for nested try_for in our current test suite, so that we can indeed see that it works fine in practice?

I don’t think there’s any place currently where I’d like to put it in at the moment. It could fit here:

--- a/features/support/helpers/exec_helper.rb
+++ b/features/support/helpers/exec_helper.rb
@@ -12,7 +12,7 @@ class VMCommand

   def VMCommand.wait_until_remote_shell_is_up(vm, timeout = 30)
     try_for(timeout, :msg => "Remote shell seems to be down") do
-      Timeout::timeout(3) do
+      try_for(3) do
         VMCommand.execute(vm, "echo 'hello?'")
       end
     end

but let’s not mess with that any more. Perhaps we want a meta.feature that tests certain more complex helper functions, like this one?

> If you’ve tested it already, I’m happy to take your word for it, and then feel free to merge this branch yourself. Also, I don’t mean to make you spend too much time on this, and perhaps anyway it’s worth postponing to 1.4.1, given how your plate for 1.4 is pretty much full already.

I’m fairly convinced by my testing of this so please go ahead.

#11 Updated by intrigeri 2015-05-02 04:03:21

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

Applied in changeset commit:39f48cf077f8922cd7b57bdc2e8e538256a8b5a1.

#12 Updated by intrigeri 2015-05-02 04:04:52

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

Thanks for the clarification :)

#13 Updated by BitingBird 2015-05-12 18:40:28

• Status changed from Fix committed to Resolved