-
-
Notifications
You must be signed in to change notification settings - Fork 518
[sidekiq] better and isolated rails specs #2723
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
c605a1f to
f88a5bb
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2723 +/- ##
=======================================
Coverage 90.28% 90.28%
=======================================
Files 131 131
Lines 5258 5258
=======================================
Hits 4747 4747
Misses 511 511
🚀 New features to boost your workflow:
|
|
We generally default enable integrations that are widely used. Sentry SDK design is not opt-in in most cases. What errors specifically does the patch cause? |
Now rails spec for sidekiq runs always as an isolated spec.
f88a5bb to
7efa6bc
Compare
@sl0thentr0py see recent commit. Basically after moving the conditional integration test to the isolated dir, for whatever reason, redis instrumentation started kicking in in specs where it previously did not, resulting in more spans in events. |
ffb555e to
d54d9b5
Compare
|
|
||
| span = transport.events.last.spans.detect { |span| span[:op] == "queue.publish" } | ||
| expect(span[:op]).to eq("queue.publish") | ||
| expect(span[:data]['messaging.destination.name']).to eq('default') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ty, makes sense!
This updates sidekiq spec setup to be inline with other projects by adding
spec:isolatedgroup and movingrails_spec.rbthere because it is meant to be run in isolation wheresentry-railsis required.While working on this I also discovered that there's spec flakiness here due to the fact that
redispatch is enabled by default and spec require order would make some specs randomly fail under Sidekiq <= 6.5. I fixed it by removingredisfrom default patches in the test config but I am wondering why this patch is enabled be default?#skip-changelog