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

Remove Sidekiq::Testing.inline! #3495

Closed
nateberkopec opened this Issue Jun 2, 2017 · 17 comments

Comments

9 participants
@nateberkopec
Contributor

nateberkopec commented Jun 2, 2017

Semi-troll incoming! I realize this is unlikely, but wanted to get your thoughts.

I propose that Sidekiq::Testing.inline! is almost entirely misused, and just allows people to write slow, lazy tests.

The nightmare scenario is this: user puts Sidekiq jobs into ActiveRecord callbacks, then uses something like factory_girl and creates hundreds of records and thus sidekiq jobs on every test. There is no feedback that tons of Sidekiq jobs are being executed besides the tests being slow. Now the user's unit tests take minutes to complete.

Now, before you say "oh that's just a dumb user, provide sharp tools, etc etc" just look at how people actually use inline!.

Is there any use-case for inline which cannot be replaced by SomeWorker.drain?

Proposals:

  • If inline is used, provide some logger feedback that jobs are being executed, and maybe how long they're taking. Sort of like the feedback provided when using delay on instance methods.
  • Remove inline testing entirely.
  • Remove the inline! method and only allow the block form of inline, which should eliminate some of it's misuse (though people will still use around blocks I bet)
@mperham

This comment has been minimized.

Owner

mperham commented Jun 2, 2017

Compelling pitch. I have never used inline myself, I think because it always had a vague smell to it. Historically there's been two reasons why people have used it outside of testing:

  1. Don't have to run separate Sidekiq and Redis processes in development.
  2. By running inside puma, they got Sidekiq::Worker code reloading (not necessary with Rails 5 anymore)
@nateberkopec

This comment has been minimized.

Contributor

nateberkopec commented Jun 2, 2017

Don't have to run separate Sidekiq and Redis processes in development.

Ah, ok, that explains all the usage in config/initializers. I think that's a valid use case. I wouldn't do it, but I get it. Any solution provided here could be mis-used for testing, though, so I don't know how would satisfy one and not the other. Maybe just a RACK_ENV-based warning.

@mperham

This comment has been minimized.

@nateberkopec

This comment has been minimized.

Contributor

nateberkopec commented Jun 2, 2017

Yeah I guess what I'm proposing is the opposite of that warning - using inline! in tests. You could also move inline! outside of the Testing class.

@mperham

This comment has been minimized.

Owner

mperham commented Jun 2, 2017

What about a Sidekiq::Development.inline! API + removing the support for block-less Sidekiq::Testing.inline!?

Side note: Sidekiq needs an explicit API to get the current environment.

@nateberkopec

This comment has been minimized.

Contributor

nateberkopec commented Jun 2, 2017

That would definitely be better. Add some wiki stuff about why inline is bad/dangerous in tests, and you're probably set.

@j3pic

This comment has been minimized.

j3pic commented Jun 22, 2017

This is an absolutely horrible idea! I'm trying to get Testing.inline! to work. The instructions in the wiki simply don't work. All the Sidekiq jobs go to Redis instead.

I'm working on a codebase that has exactly the nightmare scenario described here (ActiveRecord callbacks spawning Sidekiq jobs), and I can't tell if the callbacks even work in development unless I run the whole Sidekiq server locally (and if an error happens in the Sidekiq server, it'll be very difficult to debug)! There isn't even a way to do something like Sidekiq::Queue.pop.perform because the things in the queue are just these incomprehensible serialized monstrosities (that aren't serialized with Marshal.dump so I have no idea how to deserialize them) that don't look like they even have the parameters that are supposed to be being passed to the jobs.

@thbar

This comment has been minimized.

Contributor

thbar commented Jun 28, 2017

@mperham Just chiming in to state I personally like what you proposed. FWIW, my use cases with inline are:

  • Mostly, using the explicit block form in in-browser integration tests: this kind of documents the fact that something is expected to be going on in the background, without the need to be super specific with regard to which types of jobs are running
  • Running inline sidekiq jobs inside rake db:seed (using Sidekiq::Development.inline! in the future instead of the current version), which is convenient for data-sync heavy apps I work on (this is more a hack, but a very convenient one!)

Apart from that, yes, the use of ActiveRecord callbacks are one of the biggest pains faced by the "maintainer jumping on an unknown codebase", sadly 😄.

@ryansch

This comment has been minimized.

Collaborator

ryansch commented Jul 19, 2017

We use the block form of inline! quite a bit in our feature specs.

Example:

Sidekiq::Testing.inline! do
  WorkerThatSpawnsOtherJobs.drain
end
@mperham

This comment has been minimized.

Owner

mperham commented Aug 28, 2017

Is someone interested in providing a PR which logs inline job runs with timing? I don't want to remove APIs at this point but more information (even at DEBUG level) would be useful.

@ayanko

This comment has been minimized.

ayanko commented Aug 30, 2017

user puts Sidekiq jobs into ActiveRecord callbacks

That is development/architecture fault of this user.

Why we should remove Sidekiq functionality due to this "issue" ?

Please leave Sidekiq::Testing.inline! - we want to have full integrations tests in capybara spec/features

@mikegee

This comment has been minimized.

Contributor

mikegee commented Aug 30, 2017

we want to have full integrations tests in capybara spec/features

Its arguably not "full" integration if you don't push the work to Redis and run them from a Sidekiq worker process.

@mzagaja

This comment has been minimized.

mzagaja commented Aug 31, 2017

Using inline! in development is super handy. I haven't tried using it in my actual tests but after reading the wiki it does seem like the first approach of using drain is a bit more comprehensive.

@seanfcarroll

This comment has been minimized.

seanfcarroll commented Sep 17, 2017

I vote for leaving it in: it is useful in both dev and tests.

@nateberkopec

This comment has been minimized.

Contributor

nateberkopec commented Sep 18, 2017

I don't want to remove APIs at this point but more information (even at DEBUG level) would be useful.

Something similar to the warning that was added in Sidekiq 5 re: job size arguments would work.

# if we are using Sidekiq::Testing.inline! 
::Sidekiq.logger.debug { "#{@target}.#{name} took #{time_elapsed}" }

I still advocate for the original solution of Sidekiq::Development.inline! API + removing the support for block-less Sidekiq::Testing.inline!.

@mperham

This comment has been minimized.

Owner

mperham commented Sep 29, 2017

We have 5 years of codebases using Sidekiq and I don't want to break APIs without a clean, documented migration path and a very good reason. Are there specific changes anyone recommends and would be willing to prototype? Without a specific proposal, I'm going to close this issue; we all agree -- "this could be better" -- but what is better enough to justify the migration pain?

@mperham

This comment has been minimized.

Owner

mperham commented Dec 14, 2017

When in doubt, don't poke a sleeping bear. Too much inertia here so we'll leave it alone.

@mperham mperham closed this Dec 14, 2017

@thebadmonkeydev thebadmonkeydev referenced this issue Dec 15, 2017

Merged

Add testing module #7

3 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment