Codecov Report

Attention: Patch coverage is 84.21053% with 24 lines in your changes missing coverage. Please review.

Project coverage is 98.82%. Comparing base (03293ef) to head (69e41db).

@@            Coverage Diff             @@
##           master    #2380      +/-   ##
==========================================
- Coverage   98.97%   98.82%   -0.15%     
==========================================
  Files         231      233       +2     
  Lines       15094    15244     +150     
==========================================
+ Hits        14939    15065     +126     
- Misses        155      179      +24     

There are some tests failing under older rubies/rails + a couple of rubocop offenses to fix. Oh and conflict with CHANGELOG still has to be resolved.

Other than that, LGTM. I tested it out locally and it seems to work well.

@solnic
I've adjusted rubocop.
It seems the failing specs are related to Rails 6.0.x release. I just scanned over the changelog for activesupport, and I see nothing related there - Quick test says, hit is not in the payload from the AS notification.

I can drill down into why, but it seems to be out of "our hands".

I've adjusted rubocop.

Awesome :)

It seems the failing specs are related to Rails 6.0.x release. I just scanned over the changelog for activesupport, and I see nothing related there - Quick test says, hit is not in the payload from the AS notification.

I can drill down into why, but it seems to be out of "our hands".

I think it's perfectly fine to skip it in Rails 6 and add a note to CHANGELOG (and later on in the docs) that it's Rails 7+ only feature.

I think it's perfectly fine to skip it in Rails 6 and add a note to CHANGELOG (and later on in the docs) that it's Rails 7+ only feature.

Seems more like a regression in 6.0.x. Works in 5.x and 6.1+
We can skip it, no problem.

I think it's perfectly fine to skip it in Rails 6 and add a note to CHANGELOG (and later on in the docs) that it's Rails 7+ only feature.

Seems more like a regression in 6.0.x. Works in 5.x and 6.1+ We can skip it, no problem.

I was unable to find anything obvious in Rails 6.0.x releases. I have skipped the spec, and rubocop as well as rspec is passing for me locally. @solnic

@frederikspang thanks for addressing remaining issues - could you update your branch to latest master? Coverage should be OK after rebasing as we now merge coverage results from all test runs :)

@frederikspang thanks for addressing remaining issues - could you update your branch to latest master? Coverage should be OK after rebasing as we now merge coverage results from all test runs :)

@solnic

Squashed, and rebased :)

@solnic I see coverage is kind of misleading in the _spec.rb file.. Rails 8 is not released yet, and is not in the test matrix.
The specs I wrote there are an "improvement" that requires my Rails PR to be release - It's been merged into 8.0, but not backmerged. (Instrumentation of all cache stores).

Spec coverage should be fixed by #2437 I see :)

can I merge this now @solnic?

can I merge this now @solnic?

I merged it :) This PR made me think we could consider adding Rails HEAD to the test matrix - WDYT? Not necessarily as a strict CI step, ie allow it to issue a warning if some specs are failing instead of making CI fail. This way we could catch some issues early on.

can I merge this now @solnic?

I merged it :) This PR made me think we could consider adding Rails HEAD to the test matrix - WDYT? Not necessarily as a strict CI step, ie allow it to issue a warning if some specs are failing instead of making CI fail. This way we could catch some issues early on.

Great idea! Might throw in Ruby head as well - or just add it "early" for the preview releases

This PR made me think we could consider adding Rails HEAD

We used to do both Rails head and Ruby head. But eventually they just stayed broken most of the time and nobody would check them. More importantly, many times the failures were actually capturing Rails/Ruby/dependencies' bugs, not the SDK's. But we usually only found out after spending hours on it.

So I've adapted the approach to just update the CI matrix and tests when there are release candidates (e.g. #2444). It's more economical to fix issues at once when things have mostly settled.

@st0012 yes this makes sense, thanks for the insights!