Bug #9223

Improve the semantics of try_for

Added by anonym 2015-04-10 13:28:35 . Updated 2017-12-15 02:10:46 .

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

0%

Feature Branch:
Type of work:
Code
Blueprint:

Starter:
Affected tool:
Deliverable for:

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.

Subtasks

History

#1 Updated by anonym 2015-04-10 13:29:18

  • Assignee set to kytv

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

#2 Updated by intrigeri 2015-04-11 09:31:03

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 2015-04-24 13:58:59

  • 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 2015-09-11 03:54:53

  • 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 2015-09-14 10:24:28

  • 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 2015-09-23 09:13:26

#7 Updated by anonym 2017-03-05 09:34:10

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 2017-12-15 02:10:46

  • Status changed from Confirmed to In Progress

Applied in changeset commit:cb816375ee0244388d1799f4cfdcf7096e6a7568.