Remove workaround for rspec-mocks issue and bump RSpec to 3.12 #2346
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
On Ruby 2.7 and some RSpec versions, we were getting failures due to:
```
Datadog::Tracing received :trace with unexpected arguments
expected: (#<Double "span_name">, {})
got: (#<Double "span_name">)
Diff:
@@ -1 +1 @@
-[#<Double "span_name">, {}]
+[#<Double "span_name">]
```
There seems to be a weird incompatibility between RSpec and Ruby 2.7.
To solve this, I instead improved the test by passing an actual
argument. This works around the RSpec incompatibility and actually
improves coverage because previously we didn't actually test that
the `options` were correctly propagated (e.g. if we changed the
production code to set the options to always empty, the tests would
still pass).
Codecov Report
@@ Coverage Diff @@
## master #2346 +/- ##
==========================================
- Coverage 98.31% 98.30% -0.01%
==========================================
Files 1100 1100
Lines 58332 58331 -1
==========================================
- Hits 57347 57344 -3
- Misses 985 987 +2
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
TonyCTHsu
approved these changes
Nov 2, 2022
| @@ -11,7 +11,7 @@ | |||
| subject(:subscription) { described_class.new(span_name, options, &block) } | |||
|
|
|||
| let(:span_name) { double('span_name') } | |||
| let(:options) { {} } | |||
| let(:options) { { resource: 'dummy_resource' } } | |||
There was a problem hiding this comment.
Is this change also necessary?
There was a problem hiding this comment.
Yes -- see 158f165 for why :)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?:
This PR removes the workaround added in #2327 and #2337 to lock us out of using the latest version of rspec-mocks.
It then bumps the minimum version of RSpec used to the latest 3.12 series (which still works on all Ruby versions we support).
Motivation:
I was planning on reporting the RSpec bug we saw in #2327 and #2337 upstream, but then could no longer reproduce the issue in the latest version.
When I pushed the upgrade to 3.12 to CI, I still saw an incompatibility with Ruby 2.7, but that seems to have already been reported upstream rspec/rspec-mocks#1492 and because it only happened in a couple of places I just tweaked the code to avoid triggering the issue.
Additional Notes:
(Nothing to add)
How to test the change?:
Validate that CI is still green.