Add newest test apps #663
Add newest test apps #663
Conversation
spec/spec_helper.rb
Outdated
| config.around do |example| | ||
| Timeout.timeout(test_timeout, &example) | ||
| end | ||
|
|
There was a problem hiding this comment.
I def want that ... just set the timeout to something high for the slow tests
There was a problem hiding this comment.
Travis already has built-in timeout detection, not sure about Appveyor. I'll add it back anyways.
spec/rails_spec.rb
Outdated
| result = io.read | ||
| end | ||
| raise "FAILED #{command}\n#{result}" if $?.success? == !!options[:fail] | ||
| result | ||
| end | ||
|
|
||
| %w(rails42 rails50).each do |rails| | ||
| %w(rails42 rails50 rails51 rails52).each do |rails| |
There was a problem hiding this comment.
maybe drop 5.0, supporting 2 latest could be good enough ...
There was a problem hiding this comment.
Up to you, just confirm and I'll drop 4.2 and 5.0 then.
|
travis has a global timeout detection, but not per testcase
…On Thu, Oct 25, 2018 at 7:35 PM David Rodríguez ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In spec/spec_helper.rb
<#663 (comment)>
:
> @@ -188,12 +188,6 @@ def setup_runtime_log
config.raise_errors_for_deprecations!
- # sometimes stuff hangs -> do not hang everything
- config.include(Module.new {def test_timeout;30;end })
- config.around do |example|
- Timeout.timeout(test_timeout, &example)
- end
-
Travis already has built-in timeout detection, not sure about Appveyor.
I'll add it back anyways.
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#663 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAsZyrHYtnM5f2LuC6CohyVyThD5Idgks5uonTggaJpZM4X6FgG>
.
|
|
I see. Well, I really hope that timeout never triggers and we no longer get anymore hangs... :) And that timeout's risks do not apply to us 😅. I'll add it back tomorrow! |
``` rails _5.1.6_ new spec/fixtures/rails51 --skip-listen --skip-spring ```
``` rails _5.2.1_ new spec/fixtures/rails52 --skip-listen --skip-spring --skip-bootsnap ```
With `bin/rails g model User` and each one.
Through `bin/rails db:migrate`.
|
@grosser I applied your requests, this should be ready now! |
spec/spec_helper.rb
Outdated
| @@ -189,7 +189,7 @@ def setup_runtime_log | |||
| config.raise_errors_for_deprecations! | |||
|
|
|||
| # sometimes stuff hangs -> do not hang everything | |||
| config.include(Module.new {def test_timeout;30;end }) | |||
| config.include(Module.new {def test_timeout;300;end }) | |||
There was a problem hiding this comment.
why do all tests need 5 minutes now ... can we get this back to everything gets 30s and the rails ones get 5 minutes ?
There was a problem hiding this comment.
These tests were also sometimes timing out prior to this PR, and they are also sometimes passing without the modification in the timeout. I'm just increasing the timeout so that they pass more reliably. The new value, I took it from the unused let being removed in 326d08f. Instead, I'll dig into why that let became unused and try to restore it.
There was a problem hiding this comment.
Ah ok, I misunderstood this. I see what you're doing now. Let me update it.
There was a problem hiding this comment.
I'm unsure which value to use, though, I guess... 400? Or I can leave it as it is and just restart builds the few times they timeout?
I proposed to just remove the timeout because from all the times I have run the specs, I haven't seen a single hang, just some slow specs for which the timeout is not appropriate.
|
I saw hanging when creating infinite loops/deadlocks by accident, which are
hard to debug when all you get is travis saying "terminated build"
would prefer 30s globally and bump it to what makes sense for whatever test
needs more
…On Fri, Oct 26, 2018 at 8:42 AM David Rodríguez ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In spec/spec_helper.rb
<#663 (comment)>
:
> @@ -189,7 +189,7 @@ def setup_runtime_log
config.raise_errors_for_deprecations!
# sometimes stuff hangs -> do not hang everything
- config.include(Module.new {def test_timeout;30;end })
+ config.include(Module.new {def test_timeout;300;end })
I'm unsure which value to use, though, I guess... 400? Or I can leave it
as it is and just restart builds the few times they timeout?
I proposed to just remove the timeout because from all the times I have
run the specs, I haven't seen a single hang, just some slow specs for which
the timeout is not appropriate.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#663 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAsZ8q739kjxpMo4KARrG7pwXCF4Aqxks5uoy2CgaJpZM4X6FgG>
.
|
|
Maybe we can instead enable the RSpec documentation formatter in CI, so one can easily see where the hang happened and debug it. If not, I'll bump the timeout in |
|
I like the dots though ... less clutter :D
the timeouts also serve as a kind of sanity check in case someone adds a
new slow test or makes an existing test super slow
…On Fri, Oct 26, 2018 at 8:49 AM David Rodríguez ***@***.***> wrote:
Maybe we can instead enable the RSpec documentation formatter in CI, so
one can easily see where the hang happened and debug it. If not, I'll bump
the timeout in rails_spec.rb to 400 seconds, which should be enough I
guess.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#663 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAsZzwDG7PdzMVimnis2Ovgu5eDjkEXks5uoy8UgaJpZM4X6FgG>
.
|
Let's give it one more minute and see if it starts to never timeout.
|
Fair enough, I increased it to 360 (one more minute) only for the rails specs 👍. |
|
Ended up increasing it to 420 seconds, because with 360 one entry timed out. |
|
thx! |
We were not testing against Rails 5.1 nor Rails 5.2. This PR adds fixture apps for those Rails versions, and test against them.