Fix some thread-leaking tests #1541
Conversation
| @@ -225,6 +226,8 @@ | |||
| test_class.configure do |c| | |||
| c.runtime_metrics.statsd = new_statsd | |||
| end | |||
|
|
|||
| expect(old_statsd).to_not have_received(:close) | |||
There was a problem hiding this comment.
I've finally found a case where expect(...).to receive(...) is not equivalent to expect(...).to have_received(...).
In this specific case, old_statsd.close is invoked in an after block, which trips expect(...).to receive(...), but not expect(...).to have_received(...).
There was a problem hiding this comment.
Good point, I had never considered that the have_received has a temporal component, it's "this thing has/hasn't happened so far -- doesn't say anything about the future". (But to be honest I never use it so maybe that's why I hadn't run into this before :P)
Although -- may I suggest moving the expectation to the it? It's a bit weird to have it in the before, since the it specifically states "...but does not close the old Statsd".
Codecov Report
@@ Coverage Diff @@
## master #1541 +/- ##
=======================================
Coverage 98.23% 98.23%
=======================================
Files 878 878
Lines 42269 42279 +10
=======================================
+ Hits 41523 41534 +11
+ Misses 746 745 -1
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
To be honest, the messiness in the configuration_spec betrays how this weird mix of semi-integration-semi-unit test that says it's testing Configuration but it's actually testing Components half of the time is not a good place to be in.
I think getting a sane configuration_spec will be a good indicator for the success of your plans to refactor the component initialization :)
| @@ -225,6 +226,8 @@ | |||
| test_class.configure do |c| | |||
| c.runtime_metrics.statsd = new_statsd | |||
| end | |||
|
|
|||
| expect(old_statsd).to_not have_received(:close) | |||
There was a problem hiding this comment.
Good point, I had never considered that the have_received has a temporal component, it's "this thing has/hasn't happened so far -- doesn't say anything about the future". (But to be honest I never use it so maybe that's why I hadn't run into this before :P)
Although -- may I suggest moving the expectation to the it? It's a bit weird to have it in the before, since the it specifically states "...but does not close the old Statsd".
| let(:old_statsd) { instance_double(Datadog::Statsd, close: nil) } | ||
| let(:new_statsd) { instance_double(Datadog::Statsd, close: nil) } | ||
|
|
||
| before do | ||
| # Since its being reused, it should not be closed. | ||
| expect(old_statsd).to_not receive(:close) | ||
| allow(old_statsd).to receive(:close) |
There was a problem hiding this comment.
I believe the allow can be removed, since adding close: nil to the double implicitly states that it is allowed.
| let(:statsd) { instance_double(Datadog::Statsd) } | ||
|
|
||
| before do | ||
| expect(statsd).to_not receive(:close) | ||
| allow(statsd).to receive(:close) |
There was a problem hiding this comment.
Minor: Maybe move the allow to be part of the double setup, like the previous test?
|
|
||
| test_class.configure do |c| | ||
| c.runtime_metrics.statsd = statsd | ||
| c.diagnostics.health_metrics.statsd = statsd | ||
| end | ||
|
|
||
| test_class.configure { |_c| } | ||
|
|
||
| expect(statsd).to_not have_received(:close) |
There was a problem hiding this comment.
I think this should also be part of the it, since this is the behavior we're after in this test (reusing and not closing).
| let(:tracer) { Datadog::Tracer.new } | ||
|
|
||
| before do | ||
| expect(tracer).to_not receive(:shutdown!) | ||
| allow(tracer).to receive(:shutdown!) |
There was a problem hiding this comment.
Could we use a double of the tracer here, like we do for the statsd above?
While trying to debug leaking threads associated with
dogstatsd-ruby, I noticed that some of our tests are leaking threads, which makes it hard to use our test suite to help us debug the issue.This PR addresses leaky threads associated with
spec:maintests.A run of
spec:mainwon't be 100% clean yet, as the dreaded./spec/ddtrace/workers/*tests are still leaking, but they are pretty hard to clean, so have to be addressed separately.