Ensure clean environment for RSpec integration tests #1420
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1420 +/- ##
=======================================
Coverage 98.13% 98.14%
=======================================
Files 771 771
Lines 36929 36908 -21
=======================================
- Hits 36242 36222 -20
+ Misses 687 686 -1
Continue to review full report at Codecov.
|
marcotc
changed the title
| # Yields to a block in a new RSpec global context. All RSpec | ||
| # test configuration and execution should be wrapped in this method. |
There was a problem hiding this comment.
can this be done on an around block to avoid having to do it manually for each test?
There was a problem hiding this comment.
To be honest, I like the explicitness -- we're running RSpec to test RSpec so having clear boundaries on when transitions between the nested under-test RSpec and the top-level running-test RSpec happen I think is good for avoiding confusion.
There was a problem hiding this comment.
Adding it to around limits the use of the host RSpec test suite features inside the test example.
| spec = with_new_rspec_environment do | ||
| RSpec.describe 'some test' do | ||
| it 'foo' do; end | ||
| end.tap(&:run) |
There was a problem hiding this comment.
Minor -- we seem to be attaching run to every call to with_new_rspec_environment. What do you think of automatically calling run on the result of yield inside the helper method?
There was a problem hiding this comment.
I thought of doing that, but chose against it as it gives a seemly minor code simplification at the expense of removing flexibility from with_new_rspec_environment users.
Having with_new_rspec_environment solely "give you a clean rspec environment" made more sense to me while writing these tests.
|
|
||
| expect(spans.count).to eq(1) | ||
| span = spans.first | ||
| expect(span.span_type).to eq(Datadog::Ext::AppTypes::TEST) |
There was a problem hiding this comment.
These implicit helpers get me confused every time, because they show up from nowhere -- I need to grep the codebase to understand where they coming from.
It's a bit minor, but I'd really like if we could call them explicitly TracerHelpers.span (it's really obvious where it's coming from) or we had an explicit include TracerHelpers in this file.
There was a problem hiding this comment.
TracerHelpers.span everything is too verbose, include TracerHelpers could be nice.
When running a single example in the RSpec integration test suite (e.g.
rspec --example 'RSpec hooks creates span for example'), I noticed that theses test would fail, but while running the whole file it would pass.This happened because the RSpec test context created for testing was not completely isolated from the
ddtraceRSpec test runner. We already isolatedRSpec.configuration, but we forgot to also isolateRSpec.world. This PR does that. This behaviour became more apparent when working addressing issues around configuration leaks between tests.Also, this PR refactors the code in
spec/ddtrace/contrib/rspec/instrumentation_spec.rb, as there were a few unused variables and manual span tracking (which is already provided by https://github.com/DataDog/dd-trace-rb/blob/master/spec/support/tracer_helpers.rb).