Error Lifecycle Tests

[jjb]

Here is a test suite that is structured in a way so as to be able to easily test behavior when a timeout error is raised.

Feedback on style, approach, anything is very welcome!

[hsbt]

Don't put target code in test/lib directory. It's only used for extension of test-unit.

[jjb]

done

[hsbt]

TestTimeout is already used at https://github.com/ruby/timeout/blob/master/test/test_timeout.rb#L5. You should rename this.

[jjb]

done

[jjb]

got this failure in only truffleruby on macos after that change

https://github.com/jjb/timeout/actions/runs/5439839222/jobs/9892180910

it succeeded on a re-run: https://github.com/jjb/timeout/actions/runs/5439839222/jobs/9892337056

maybe it's a fluke caused by the CI environment

but maybe it's an indication of a race condition in the code?

[eregon]

I don't see the point of merging this, we already have similar tests, including that rescue Exception catches the exception from Timeout.timeout.
One test is enough to test that Timeout.timeout is using a Ruby exception and not a throw.

[eregon]

All of these UNDESIRED? are expected and simply testing whether one can rescue an exception from Timeout.
If you rescue an exception, you are responsible for handling it.
If you don't do anything in the rescue and rescue Exception, it's a bug of that code.
As Jeremy said in #30 (comment)

[eregon]

Regarding #7 (comment)

Yes, more tests are welcome.

I expected:

I'll redo them so that they are more in the style of the current tests

i.e., similar to existing tests, not something quite different. Also new tests should cover something which is not already tested, otherwise it's duplicated tests which is more work to maintain.

[jjb]

Thanks for taking a look! I understand your reasoning, but maybe I should have done a better pitch in my initial PR, so I'll try now.

I don't see the point of merging this, we already have similar tests, including that rescue Exception catches the exception from Timeout.timeout.
One test is enough to test that Timeout.timeout is using a Ruby exception and not a throw.

Gotcha. My thinking was, timeout, for something that is so widely used, is a fairly complicated, sometimes unintuitive tool, with a mix of opinions and compromises. These tests are an easy way to map out the lifecycle and test the whole space without having to reinvent the wheel for each test, and to be confident that the whole space is covered. The space is 6x9, so 54 behaviors are being tested. So, there are certainly a lot of things being tested here not currently in the suite. Of course, 6x6 of these (core_assertions) are the same in every scenario, so maybe that's what makes this seem unremarkable. But it's nice to have those covered. When I first put it together it was perhaps more interesting, because it called out the weird catch/throw scenario.

Also, I thought it was interesting to just call out some behaviors explicitly, which might spur discussion or documentation. Ensure blocks have infinite time to finish - that's a surprising behavior for a tool used to time-limit code execution, I think it's not documented anywhere. Sometimes code will time out but not raise. This is covered by an existing test, for only 1 of the 4 scenarios which these tests cover.

I forgot to try this before but just thought, this suite could probably be extended to cover the scenario from Charles Nutter's classic screed, a timeout happening inside of an ensure block. Having this covered by a test might inspire someone to experiment with a reimplementation which fixes it (seems impossible now, but who knows what ruby 4 or some exotic ruby implementation in the future might enable).

All of these UNDESIRED? are expected and simply testing whether one can rescue an exception from Timeout.
If you rescue an exception, you are responsible for handling it.
If you don't do anything in the rescue and rescue Exception, it's a bug of that code.
As Jeremy said in #30 (comment)

Sure, as noted elsewhere those were just there for discussion, was going to flatten them into the other expectations after getting feedback on the overall approach.

If you got this far, thanks for reading! I understand if you still don't want to merge - thanks for maintaining timeout! You are doing the lord's work.

[jjb]

I started experimenting with this and decided to try starting from scratch, and I came up with this alternative implementation of Timeout which does have the behavior I want 😄 I have almost the entire Timeout test suite passing on it.

@eregon I used Concurrent::ScheduledTask for it, which I noticed you're the most recent committer on 😄

https://github.com/jjb/time_limit/blob/main/readme.md
https://github.com/jjb/time_limit/blob/main/time_limit.rb