Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Unit spec timeout #15

Merged
merged 2 commits into from

4 participants

@dkubb
Collaborator

This branch adds a 100ms timeout on all unit tests. This is mainly to open up discussion on whether or not limiting the unit test runtime will result in better tests.

@mbj, @snusnu, @solnic I would love to get input from you on this especially.

dkubb added some commits
@dkubb dkubb Add a timeout that causes a spec failure on long running unit tests
* A proper unit test should always execute in less time than 1/10th
  of a second, and more likely 1/100th of a second. Having an upper
  bounds with a (ridiculously) large limit, that is still commonly
  exceeded, will ensure that out unit tests will remain proper unit
  tests and not devolve into integration tests.

  Over time we can (and should!) work to reduce this threshold
  even further.
7f65e8c
@dkubb dkubb Update flay and reek thresholds 4e46d1c
@solnic
Collaborator

I don't think this should be guarded by another constraint. Also 100ms is a crazy constraint. Virtus specs run in ~200ms and it's quite fast IMHO. Frankly, everything below 1second is really fast enough. I don't think that we should be bothered by such metric. Also, there are cases where a spec suite would run slower because some arbitrary process in the system took some resources and slowed things down. I don't want to see failures bkz of that.

I think this is really unnecessary. I don't want to waste my time tweaking my spec suite so that it's running 100ms faster or even 500 ms faster. It's amount of time I don't notice, I don't care about this at all.

When I see my unit spec suite running for longer than, let's say 1-2 seconds it will be alarming that I either do something wrong or my library does too much. This doesn't change the fact that I don't want my specs to fail because of that. It's just very strict and "unfriendly".

@mbj
Owner
mbj commented

@solnic I think you got dkubb wrong this change is a timeout per spec / example, not for the full suite.

@dkubb I think it is an interesting change. But maybe we should measure the time the CPU spend on the task instead of realtime? This results in far less false positives.

@snusnu
Collaborator

@dkubb I'm fine with that limit per unit test. It makes sense to limit the time a single unit test should take. I agree that such a limit would most probably only be violated if the actual test is in fact an integration test, and we should catch those (and rewrite or move them elsewhere). I also agree with @mbj that we should probably measure CPU time as compared to realtime.

@dkubb
Collaborator

@solnic this timeout is per-example. It would only catch an example that was obviously doing too much work to be called a unit test.

@mbj do you have any ideas on how we could do a timeout based on CPU time? If we use something like benchmark we can get the actual time, but only after the spec finishes. With Timeout we can kill the spec before it takes too long.

@solnic
Collaborator
@dkubb
Collaborator

There doesn't appear to be any strong objections to this so I'm going to merge it in. If this creates any problems we can always revert it, or tweak the implementation.

@dkubb dkubb merged commit c078c5b into master

1 check passed

Details default The Travis build passed
@dkubb dkubb deleted the unit-spec-timeout branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 6, 2013
  1. @dkubb

    Add a timeout that causes a spec failure on long running unit tests

    dkubb authored
    * A proper unit test should always execute in less time than 1/10th
      of a second, and more likely 1/100th of a second. Having an upper
      bounds with a (ridiculously) large limit, that is still commonly
      exceeded, will ensure that out unit tests will remain proper unit
      tests and not devolve into integration tests.
    
      Over time we can (and should!) work to reduce this threshold
      even further.
  2. @dkubb
This page is out of date. Refresh to see the latest.
Showing with 19 additions and 2 deletions.
  1. +4 −2 config/reek.yml
  2. +1 −0  lib/devtools.rb
  3. +14 −0 lib/devtools/project.rb
View
6 config/reek.yml
@@ -40,7 +40,8 @@ LongYieldList:
max_params: 0
NestedIterators:
enabled: true
- exclude: []
+ exclude:
+ - Devtools::Project#self.timeout_unit_tests
max_allowed_nesting: 1
ignore_iterators: []
NilCheck:
@@ -60,7 +61,8 @@ TooManyMethods:
max_methods: 15
TooManyStatements:
enabled: true
- exclude: []
+ exclude:
+ - Devtools::Project#self.setup_rspec
max_statements: 5
UncommunicativeMethodName:
enabled: true
View
1  lib/devtools.rb
@@ -1,5 +1,6 @@
require 'pathname'
require 'rake'
+require 'timeout'
require 'yaml'
# Namespace for library
View
14 lib/devtools/project.rb
@@ -27,6 +27,7 @@ def self.setup_rspec(spec_root)
require_shared_spec_files(Devtools.shared_path.join('spec'))
require_shared_spec_files(spec_root)
prepare_18_specific_quirks
+ timeout_unit_tests
self
end
@@ -54,6 +55,19 @@ def self.prepare_18_specific_quirks
end
private_class_method :prepare_18_specific_quirks
+ # Timeout unit tests that take longer than 1/10th of a second
+ #
+ # @return [undefined]
+ #
+ # @api private
+ #
+ def self.timeout_unit_tests
+ RSpec.configuration.around :file_path => UNIT_TEST_PATH_REGEXP do |example|
+ Timeout.timeout(UNIT_TEST_TIMEOUT) { example.run }
+ end
+ end
+ private_class_method :timeout_unit_tests
+
# Initialize object
#
# @param [Pathname] root
Something went wrong with that request. Please try again.