Use Sidekiq fake! instead of inline! in specs
#25369
Conversation
renchap
added
testing
|
This pull request has merge conflicts that must be resolved before it can be merged. |
mjankowski
force-pushed
the
spec-performance-sidekiq-fake
branch
from
3324f8a
to
2d0cdf5
Compare
|
This pull request has resolved merge conflicts and is ready for review. |
|
This pull request has merge conflicts that must be resolved before it can be merged. |
|
This pull request has resolved merge conflicts and is ready for review. |
mjankowski
force-pushed
the
spec-performance-sidekiq-fake
branch
2 times, most recently
from
0bd63d0
to
88520ef
Compare
|
This pull request has merge conflicts that must be resolved before it can be merged. |
mjankowski
force-pushed
the
spec-performance-sidekiq-fake
branch
from
88520ef
to
6a9669d
Compare
|
This pull request has resolved merge conflicts and is ready for review. |
|
This pull request has merge conflicts that must be resolved before it can be merged. |
mjankowski
force-pushed
the
spec-performance-sidekiq-fake
branch
from
6a9669d
to
059544c
Compare
|
This pull request has resolved merge conflicts and is ready for review. |
mjankowski
force-pushed
the
spec-performance-sidekiq-fake
branch
2 times, most recently
from
8996a63
to
47e080a
Compare
|
This pull request has merge conflicts that must be resolved before it can be merged. |
mjankowski
force-pushed
the
spec-performance-sidekiq-fake
branch
from
47e080a
to
674587b
Compare
|
This pull request has merge conflicts that must be resolved before it can be merged. |
mjankowski
force-pushed
the
spec-performance-sidekiq-fake
branch
2 times, most recently
from
55f82fb
to
5b96e5b
Compare
|
This pull request has resolved merge conflicts and is ready for review. |
|
This pull request has merge conflicts that must be resolved before it can be merged. |
mjankowski
force-pushed
the
spec-performance-sidekiq-fake
branch
from
5b96e5b
to
bfa9fba
Compare
|
This pull request has resolved merge conflicts and is ready for review. |
mjankowski
force-pushed
the
spec-performance-sidekiq-fake
branch
2 times, most recently
from
994a160
to
dc2a979
Compare
mjankowski
force-pushed
the
spec-performance-sidekiq-fake
branch
2 times, most recently
from
602837b
to
3aad08f
Compare
|
Rebased, current benchmark (local run):
Much of the coverage lines difference is around lines which are not necessarily asserted against, and just run incidentally as part of running the full job. Obviously this is useful to have in place, but it makes tracking down what needs to be added to restore the coverage a little harder. Most of the files with changed coverages are in the single digit lines changed now - https://app.codecov.io/gh/mastodon/mastodon/pull/25369/indirect-changes - and the two larger ones I think are just side effects of not having file attachments in their unit tests, etc... I guess I'm curious what the target is here? Are we close enough to merge at this point? If not, is the target to lose zero coverage (in which case I can continue to add coverage in the gaps)? Something else? Other than the coverage concern, are there any other blockers to merging here? |
|
Sorry for the late reply. I'd like to avoid any coverage regression if possible. This is the only concern I have regarding this PR, it looks fine otherwise! Looks like most of the coverage regression could be recovered by:
|
mjankowski
force-pushed
the
spec-performance-sidekiq-fake
branch
from
3aad08f
to
a25bc69
Compare
mjankowski
force-pushed
the
spec-performance-sidekiq-fake
branch
from
a25bc69
to
42ee675
Compare
mjankowski
force-pushed
the
spec-performance-sidekiq-fake
branch
from
42ee675
to
8b99843
Compare
|
Rebased to get another CodeCov run ... we're down to just 1 or 2 LOC changes in the remaining files. |
In the spirit of previous PRs about paperclip processing (#25359) and factory build (#25360) -- there are many places throughout the spec suite which are running background sidekiq jobs inline, but where the specs don't need that to happen and aren't verifying anything related to that happening.
The default mode for sidekiq testing is to run in
fakemode (makes job information accessible to specs, but doesn't actually run the jobs), but we were overriding that to run ininlinemode.This changes removes that override so that we go back to
fakemode, and adds an opt-in for examples to get the inline behavior in specs where it's actually required.In my local spec suite runs this change drops the full run from ~8:45 or so to around ~6:05 (~2:30 improvement).
One sort of awkward thing here is the mix of opting in to the inline behavior at different levels (describe, context, it) in this diff. I tried to keep this consistent at first, but there are some specs where we have what should be one-time setup code running once for every example because
before(:each)is the default. I suspect there is good opportunity here for more speedup refactoring and might grab that next ... but I decided to limit this PR to the narrow sidekiq mode speedup instead of also changing those here.