Migrate Rails errors_test to RSpec #1649
Conversation
| # Contrib minitests | ||
| # Contrib specs |
There was a problem hiding this comment.
The comment situation here was a bit messy before 🧹🧹🧹
Codecov Report
@@ Coverage Diff @@
## master #1649 +/- ##
========================================
Coverage 98.32% 98.33%
========================================
Files 901 901
Lines 43713 43853 +140
========================================
+ Hits 42980 43121 +141
+ Misses 733 732 -1
Continue to review full report at Codecov.
|
| @@ -3,7 +3,6 @@ require 'ddtrace/version' | |||
| require 'rubocop/rake_task' if Gem.loaded_specs.key? 'rubocop' | |||
There was a problem hiding this comment.
Amazing improvement to this file! :)
Speaking of cleanups, but unrelated, I've secretly been thinking about inversing the test matrix on this file, e.g. rather than going
if 2.1
run A
run B
run C
elsif 2.2
run B
run C
run D
we could go with
run A on: ["2.1"]
run B on: ["2.1", "2.2]
I think I may try to take a stab at it and see how it works in the near future :)
There was a problem hiding this comment.
The eternal struggle on slicing things up! There's also a bunch that would fit under:
run A on: ["2.5", "2.6", "2.7", "3.0"]
or
run A on: [">= 2.5"]
Always a fun problem 😄
But I agree that inverting the dimension could it more clear: if it looks more clear after the change than it is :)
There was a problem hiding this comment.
Indeed! I'll keep it in mind for my next refactoring-zen-moment :)
| expect(spans).to have(4).items | ||
| request_span, controller_span, partial_span, render_span = spans |
There was a problem hiding this comment.
Minor: Crazy idea: since it seems really common for us to assert on spans before getting and naming them, I wonder if it would make sense to add an expectation as an optional argument to spans, e.g.
request_span, controller_span, partial_span, render_span = spans(expect: have(4))or
request_span = spans(expect: have_at_least(1)).firstThere was a problem hiding this comment.
I was thinking of this while writing these. I would be nice if this just worked:
request_span, controller_span, partial_span, render_span = expect(spans).to have(4).itemsI like something like this:
spans(expect: have(4))
# or
spans(exactly: 4)
# or
spans(at_least: 4)
# or
spans(match(arbitrary_matcher))| expect(partial_span).to have_error | ||
| expect(partial_span).to have_error_type('ActionView::Template::Error') |
There was a problem hiding this comment.
This is repeated from lines 553->554
| it 'has ActionView error tags' do | ||
| is_expected.to be_server_error | ||
|
|
||
| if Gem::Version.new(Rails::VERSION::STRING) < Gem::Version.new('3.2') | ||
| expect(spans).to have(5).items | ||
| expect(spans).to have(3).items | ||
| request_span, controller_span, render_span = spans | ||
|
|
||
| expect(request_span).to have_error | ||
| expect(request_span).to have_error_type('ActionView::Template::Error') | ||
| expect(request_span).to have_error_stack | ||
| expect(request_span).to have_error_message | ||
| expect(request_span.resource).to_not eq(render_span.resource) |
There was a problem hiding this comment.
The minispec versions of this and other tests usually had stricter assertions for the request span, e.g.:
assert_equal(span_request.name, 'rails.action_controller')
assert_equal(span_request.status, 1)
assert_equal(span_request.span_type, 'web')
assert_equal(span_request.resource, 'TracingController#error_template')
assert_equal(span_request.get_tag('rails.route.action'), 'error_template')
assert_equal(span_request.get_tag('rails.route.controller'), 'TracingController')
assert_equal(span_request.get_tag('error.type'), 'ActionView::Template::Error')
assert_equal(span_request.get_tag('error.msg'), 'divided by 0')Was the simpler version we have with RSpec intended (because maybe we already have enough coverage elsewhere...?) or is this a gap?
There was a problem hiding this comment.
I removed some of it, was it is rather redundant, specially around the same top-level rack.request span, when we are asserting a nested template.
I don't think there's a meaningful loss of coverage.
| it 'traces complete stack' do | ||
| is_expected.to be_server_error | ||
|
|
||
| expect(spans).to have(4).items | ||
|
|
||
| request_span, controller_span, partial_span, template_span = spans | ||
|
|
||
| expect(request_span.name).to eq('rack.request') | ||
| expect(request_span.resource).to eq('TestController#nonexistent_partial') | ||
| expect(request_span).to have_error | ||
| expect(request_span).to have_error_type('ActionView::Template::Error') | ||
| expect(request_span).to have_error_message(include('Missing partial test/no_partial_here')) | ||
| expect(request_span).to have_error_stack | ||
|
|
||
| expect(controller_span.name).to eq('rails.action_controller') | ||
| expect(controller_span.resource).to eq('TestController#nonexistent_partial') | ||
| expect(controller_span).to have_error | ||
| expect(controller_span).to have_error_type('ActionView::Template::Error') | ||
| expect(controller_span).to have_error_message(include('Missing partial test/no_partial_here')) | ||
| expect(controller_span).to have_error_stack | ||
|
|
||
| expect(template_span.name).to eq('rails.render_template') | ||
| expect(template_span.resource).to eq('partial_does_not_exist.html.erb') # Fallback value due to missing template | ||
| expect(template_span.span_type).to eq('template') | ||
| expect(template_span.get_tag('rails.template_name')).to eq('partial_does_not_exist.html.erb') | ||
| expect(template_span.get_tag('rails.layout')).to eq('layouts/application') | ||
| expect(template_span).to have_error | ||
| expect(template_span).to have_error_type('ActionView::Template::Error') | ||
| expect(template_span).to have_error_message(include('Missing partial test/no_partial_here')) | ||
| expect(template_span).to have_error_stack | ||
|
|
||
| expect(partial_span.name).to eq('rails.render_partial') |
There was a problem hiding this comment.
Minor: It's somewhat confusing to follow the expects not being in the same order as we get from spans; e.g. I think it'd be easier to read if they go from request_span --> template_span or in the reverse direction, rather than jumping around.
There was a problem hiding this comment.
Matched order with spans response.
There was a problem hiding this comment.
The PR is much larger now because of it, but it seems easier to read 😅
This PR migrates the last (!) minitest test file to RSpec:
test/contrib/rails/errors_test.rb.All other changes are related to the removal of the last minitest: CI, Gemfile, Rakefile, DevelopmentGuide, and the removal of all support minitest files.