Project

General

Profile

Bug #9223

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

Improve the semantics of try_for

Added by anonym over 4 years ago. Updated almost 2 years ago.

Status:
In Progress
Priority:
Normal
Assignee:
Category:
Test suite
Target version:
-
Start date:
04/10/2015
Due date:
% Done:

0%

Feature Branch:
Type of work:
Code
Blueprint:
Starter:
Affected tool:

Description

To make try_for accept a "try" it wants the block to return something that can be interpreted as true (essentially anything but nil and false) which makes sense in some cases, mostly when what we are trying is to run a single command in the block, that returns a boolean value. However, quite often we're doing multiple things, and something that will lead to weird situations. For instance:

try_for(10) do
  check_that_throws_exception_on_failure_1
  check_that_throws_exception_on_failure_2 if check_that_returns_boolean_and_never_throws_an_exception
end

If check_that_returns_boolean_and_never_throws_an_exception is false, the last line in the block will evaluate to nil, so try_for always fails. That doesn't make sense. We'd have to fix it with something ugly like:

try_for(10) do
  check_that_throws_exception_on_failure_1
  check_that_throws_exception_on_failure_2 if check_that_returns_boolean_and_never_throws_an_exception
  true
end

I think it'd be better if try_for simply runs the block without checking the return value, so that the only way a "try" fails is if there's an exceptions raised. In the instances where we run something like:

try_for(10) { check_that_returns_boolean_and_never_throws_an_exception }

we then instead do:

try_for(10) { assert(check_that_returns_boolean_and_never_throws_an_exception) }

which imho is even clearer.

Associated revisions

Revision 27370365 (diff)
Added by anonym almost 3 years ago

Add missing return value to try_for().

... due to try_for()'s silly semantics.

Refs: #9223

Revision b715ce42 (diff)
Added by anonym over 2 years ago

Test suite: fixes to make 3.0~beta3 pass.

This includes:

  • fixing some mistakes in the merge conflict resolution
    28cf4852928035b7c8707ee3777d6a443a8ab5d2.
  • switching to an image in the Pidgin tests that doesn't depend on the
    /topic of -- the server resets it every
    once in a while, so we should not pretend the test suite can always
    find it.
  • Fix a problematic use of try_for (we need refs: #9223!).
  • Fix for VM.select_virtual_desktop() and VM.do_focus(): in Stretch we
    only have two virtual desktop, so do_focus() has been broken for a
    while. Also add some `sleep()`:s which sadly seem required.
  • Random Gherkin improvement.

Revision f7940073 (diff)
Added by anonym almost 2 years ago

Test suite: reinvent try_for()/retry_action as try().

Interface-wise we have:

  • try_for(t, ...) => try(timeout: t, ...)
  • retry_action(n, ...) => try(attempts: n, ...)
  • + a few minor changes (e.g. 'msg' => 'message')

The other important change is that we fix #9223, i.e. the code block
no longer must return `true` to pass; instead we provide a
`try_for_success()` for the cases where we actually want that.

Will-fix: #9223

Revision c618bc0b (diff)
Added by anonym over 1 year ago

Test suite: reinvent try_for()/retry_action as try().

Interface-wise we have:

  • try_for(t, ...) => try(timeout: t, ...)
  • retry_action(n, ...) => try(attempts: n, ...)
  • + a few minor changes (e.g. 'msg' => 'message')

The other important change is that we fix #9223, i.e. the code block
no longer must return `true` to pass; instead we provide a
`try_for_success()` for the cases where we actually want that.

Will-fix: #9223

Revision fefefecd (diff)
Added by anonym 6 months ago

Test suite: reinvent try_for()/retry_action as try().

Interface-wise we have:

  • try_for(t, ...) => try(timeout: t, ...)
  • retry_action(n, ...) => try(attempts: n, ...)
  • + a few minor changes (e.g. 'msg' => 'message')

The other important change is that we fix #9223, i.e. the code block
no longer must return `true` to pass; instead we provide a
`try_for_success()` for the cases where we actually want that.

Will-fix: #9223

Revision 706cde46 (diff)
Added by anonym 6 months ago

Test suite: reinvent try_for()/retry_action as try().

Interface-wise we have:

  • try_for(t, ...) => try(timeout: t, ...)
  • retry_action(n, ...) => try(attempts: n, ...)
  • + a few minor changes (e.g. 'msg' => 'message')

The other important change is that we fix #9223, i.e. the code block
no longer must return `true` to pass; instead we provide a
`try_for_success()` for the cases where we actually want that.

Will-fix: #9223

History

#1 Updated by anonym over 4 years ago

  • Assignee set to kytv

Any opinion on this? Reassign to me in any case.

#2 Updated by intrigeri over 4 years ago

I agree the current state of things should be improved, and it sounds like a worthy goal for 2.0 (worst case).

I think it'd be better if try_for simply runs the block without checking the return value, so that the only way a "try" fails is if there's an exceptions raised. In the instances where we run something like:

[...]

we then instead do:

> try_for(10) { assert(check_that_returns_boolean_and_never_throws_an_exception) }
> 

My only problem with this is that I fear we'll constantly forget to add the assert once out of N times. Perhaps we can make try_for consider that the block of code has failed in both cases, that is if it has returned false or thrown an exception? Or would that be confusing error handling semantics? I'm unsure what's the best design.

Also, I now realize that we have a perhaps more fundamental problem with try_for: its name makes it look very much like the good ol' try exception catching block... and indeed it catches exceptions raised by the code block, but it itself raises an exception on timeout, much unlike try, which is kinda confusing. Perhaps we should rename try_for, have it called e.g. repeat_until_success(timeout = 10) { ... }, so it's more obvious that it may raise exceptions?

#3 Updated by kytv over 4 years ago

  • Assignee changed from kytv to anonym

anonym wrote:

Any opinion on this? Reassign to me in any case.

Not particularly. This could be improved but I don't have any ideas ATM as to how to tackle it.

Perhaps we should rename try_for, have it called e.g. repeat_until_success(timeout = 10) { ... }, so it's more obvious that it may raise exceptions?

This sounds reasonable.

Reassigning per request.

#4 Updated by anonym about 4 years ago

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

intrigeri wrote:

My only problem with this is that I fear we'll constantly forget to add the assert once out of N times.

I do not worry about this at all. The only way to make a cucumber step fail is to have it throw an exception, so we are very familiar with this type of thinking.

Perhaps we can make try_for consider that the block of code has failed in both cases, that is if it has returned false or thrown an exception?

That is the current behaviour. try_for succeeds if and only if the block returns true, which implies that it didn't throw an exception either, so...

Or would that be confusing error handling semantics? I'm unsure what's the best design.

... it would keep the status quo, which imho is confusing.

Also, I now realize that we have a perhaps more fundamental problem with try_for: its name makes it look very much like the good ol' try exception catching block... and indeed it catches exceptions raised by the code block, but it itself raises an exception on timeout, much unlike try, which is kinda confusing. Perhaps we should rename try_for, have it called e.g. repeat_until_success(timeout = 10) { ... }, so it's more obvious that it may raise exceptions?

At least in Ruby there's no try keyword but I suppose it could cause confusion for people with a lot of experience with python/C++/Java/etc. I'm not sure how strong that makes your argument. Personally I switch languages constantly and rarely feel like I can necessarily rely on such conventions so I prefer when the code reads like natural language. try_for(10) do something() end reads pretty much as "try for 10 (seconds) to do something", which I quite like. repeat_until_success(10) doesn't have the same direct translation. That said, if there's some other name change suggestion that would keep the natural language vs code semantics 1:1 translation, and not confuse itself with try or other keywords/concepts common in programming languages, I'd happily consider it.

Please let me know if you completely disagree with me or not. In any case I'd like to proceed with the concern of this ticket, and consider the name change separately if you still think it's necessary.

#5 Updated by intrigeri about 4 years ago

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

Please let me know if you completely disagree with me or not.

I don't, please go ahead with what you've proposed :)

#6 Updated by anonym about 4 years ago

  • Parent task set to #10237

#7 Updated by anonym over 2 years ago

I'm now thinking that we could bake together try_for() and retry_action() (why the name inconsistency? I don't know) into a single try(), which has two basic modes, e.g. try(timeout: 60) and try(times: 3). They don't have to be mutually exclusive (which ever is reached first raises an error, e.g. TryTimeoutError and TryTimesError, respectively, both inheriting TryError) but at least one of them must be given.

I'm not entirely sure what to do with retry_tor(), but I think it should always use the times-mode, so maybe it needs a new name, but try_tor_times() doesn't sound so good.

#8 Updated by anonym almost 2 years ago

  • Status changed from Confirmed to In Progress

Applied in changeset commit:cb816375ee0244388d1799f4cfdcf7096e6a7568.

Also available in: Atom PDF