Add new `run_jobs_synchronously!` helper for tests

approved

#1

Add new run_jobs_synchronously! helper for tests

Previously if you wanted to have jobs execute in test mode, you’d have to do SiteSetting.queue_jobs = false, because the opposite of queue is to execute.

I found this very confusing, so I created a test helper called run_jobs_synchronously! which is much more clear about what it does.

diff --git a/lib/generators/plugin/templates/controller_spec.rb.erb b/lib/generators/plugin/templates/controller_spec.rb.erb
index 88935a5..c73e0a8 100644
--- a/lib/generators/plugin/templates/controller_spec.rb.erb
+++ b/lib/generators/plugin/templates/controller_spec.rb.erb
@@ -2,7 +2,7 @@ require 'rails_helper'
 
 describe <%= name %>::ActionsController do
   before do
-    SiteSetting.queue_jobs = false
+    run_jobs_synchronously!
   end
 
   it 'can list' do
diff --git a/plugins/discourse-local-dates/spec/models/post_spec.rb b/plugins/discourse-local-dates/spec/models/post_spec.rb
index 13e7e9a..9591607 100644
--- a/plugins/discourse-local-dates/spec/models/post_spec.rb
+++ b/plugins/discourse-local-dates/spec/models/post_spec.rb
@@ -3,7 +3,7 @@ require 'rails_helper'
 describe Post do
 
   before do
-    SiteSetting.queue_jobs = false
+    run_jobs_synchronously!
   end
 
   describe '#local_dates' do
diff --git a/plugins/discourse-narrative-bot/spec/discourse_narrative_bot/advanced_user_narrative_spec.rb b/plugins/discourse-narrative-bot/spec/discourse_narrative_bot/advanced_user_narrative_spec.rb
index 32a5a4e..91b0c15 100644
--- a/plugins/discourse-narrative-bot/spec/discourse_narrative_bot/advanced_user_narrative_spec.rb
+++ b/plugins/discourse-narrative-bot/spec/discourse_narrative_bot/advanced_user_narrative_spec.rb
@@ -22,7 +22,7 @@ RSpec.describe DiscourseNarrativeBot::AdvancedUserNarrative do
   let(:reset_trigger) { DiscourseNarrativeBot::TrackSelector.reset_trigger }
 
   before do
-    SiteSetting.queue_jobs = false
+    run_jobs_synchronously!
     SiteSetting.discourse_narrative_bot_enabled = true
   end
 
diff --git a/plugins/discourse-narrative-bot/spec/discourse_narrative_bot/new_user_narrative_spec.rb b/plugins/discourse-narrative-bot/spec/discourse_narrative_bot/new_user_narrative_spec.rb
index 3a7e5d8..7ecc727 100644
--- a/plugins/discourse-narrative-bot/spec/discourse_narrative_bot/new_user_narrative_spec.rb
+++ b/plugins/discourse-narrative-bot/spec/discourse_narrative_bot/new_user_narrative_spec.rb
@@ -25,7 +25,7 @@ describe DiscourseNarrativeBot::NewUserNarrative do
   let(:reset_trigger) { DiscourseNarrativeBot::TrackSelector.reset_trigger }
 
   before do
-    SiteSetting.queue_jobs = false
+    run_jobs_synchronously!
     SiteSetting.discourse_narrative_bot_enabled = true
   end
 
diff --git a/plugins/discourse-narrative-bot/spec/discourse_narrative_bot/track_selector_spec.rb b/plugins/discourse-narrative-bot/spec/discourse_narrative_bot/track_selector_spec.rb
index 0e6cb7a..4d76b06 100644
--- a/plugins/discourse-narrative-bot/spec/discourse_narrative_bot/track_selector_spec.rb
+++ b/plugins/discourse-narrative-bot/spec/discourse_narrative_bot/track_selector_spec.rb
@@ -37,7 +37,7 @@ describe DiscourseNarrativeBot::TrackSelector do
   end
 
   before do
-    SiteSetting.queue_jobs = false
+    run_jobs_synchronously!
   end
 
   describe '#select' do
diff --git a/plugins/discourse-narrative-bot/spec/user_spec.rb b/plugins/discourse-narrative-bot/spec/user_spec.rb
index 9779c24..de52f0e 100644
--- a/plugins/discourse-narrative-bot/spec/user_spec.rb
+++ b/plugins/discourse-narrative-bot/spec/user_spec.rb
@@ -13,7 +13,7 @@ describe User do
   end
 
   before do
-    SiteSetting.queue_jobs = false
+    run_jobs_synchronously!
     SiteSetting.discourse_narrative_bot_enabled = true
   end
 
diff --git a/spec/components/post_creator_spec.rb b/spec/components/post_creator_spec.rb
index 78b2d64..0e609c9 100644
--- a/spec/components/post_creator_spec.rb
+++ b/spec/components/post_creator_spec.rb
@@ -902,7 +902,7 @@ describe PostCreator do
     end
 
     it 'can post to a group correctly' do
-      SiteSetting.queue_jobs = false
+      run_jobs_synchronously!
 
       expect(post.topic.archetype).to eq(Archetype.private_message)
       expect(post.topic.topic_allowed_users.count).to eq(1)
diff --git a/spec/components/post_destroyer_spec.rb b/spec/components/post_destroyer_spec.rb
index ba12c82..d5da412 100644
--- a/spec/components/post_destroyer_spec.rb
+++ b/spec/components/post_destroyer_spec.rb
@@ -617,7 +617,7 @@ describe PostDestroyer do
 
   context '@mentions' do
     it 'removes notifications when deleted' do
-      SiteSetting.queue_jobs = false
+      run_jobs_synchronously!
       user = Fabricate(:evil_trout)
       post = create_post(raw: 'Hello @eviltrout')
       expect {
diff --git a/spec/components/post_revisor_spec.rb b/spec/components/post_revisor_spec.rb
index 4f5df63..d721692 100644
--- a/spec/components/post_revisor_spec.rb
+++ b/spec/components/post_revisor_spec.rb
@@ -608,7 +608,7 @@ describe PostRevisor do
       let(:mentioned_user) { Fabricate(:user) }
 
       before do
-        SiteSetting.queue_jobs = false
+        run_jobs_synchronously!
       end
 
       it "generates a notification for a mention" do
diff --git a/spec/integration/watched_words_spec.rb b/spec/integration/watched_words_spec.rb
index b165b17..7620237 100644
--- a/spec/integration/watched_words_spec.rb
+++ b/spec/integration/watched_words_spec.rb
@@ -163,7 +163,7 @@ describe WatchedWord do
     end
 
     it "flags on revisions" do
-      SiteSetting.queue_jobs = false
+      run_jobs_synchronously!
       post = Fabricate(:post, topic: Fabricate(:topic, user: tl2_user), user: tl2_user)
       expect {
         PostRevisor.new(post).revise!(post.user, { raw: "Want some #{flag_word.word} for cheap?" }, revised_at: post.updated_at + 10.seconds)
diff --git a/spec/jobs/pull_hotlinked_images_spec.rb b/spec/jobs/pull_hotlinked_images_spec.rb
index fc258ad..2415696 100644
--- a/spec/jobs/pull_hotlinked_images_spec.rb
+++ b/spec/jobs/pull_hotlinked_images_spec.rb
@@ -32,7 +32,7 @@ describe Jobs::PullHotlinkedImages do
 
   describe '#execute' do
     before do
-      SiteSetting.queue_jobs = false
+      run_jobs_synchronously!
       FastImage.expects(:size).returns([100, 100]).at_least_once
     end
 
diff --git a/spec/lib/upload_recovery_spec.rb b/spec/lib/upload_recovery_spec.rb
index 5e8e450..b9a6b91 100644
--- a/spec/lib/upload_recovery_spec.rb
+++ b/spec/lib/upload_recovery_spec.rb
@@ -31,7 +31,7 @@ RSpec.describe UploadRecovery do
 
   before do
     SiteSetting.authorized_extensions = 'png|pdf'
-    SiteSetting.queue_jobs = false
+    run_jobs_synchronously!
   end
 
   after do
diff --git a/spec/models/category_user_spec.rb b/spec/models/category_user_spec.rb
index 8313c0b..048a19a 100644
--- a/spec/models/category_user_spec.rb
+++ b/spec/models/category_user_spec.rb
@@ -68,7 +68,7 @@ describe CategoryUser do
 
   context 'integration' do
     before do
-      SiteSetting.queue_jobs = false
+      run_jobs_synchronously!
       NotificationEmailer.enable
     end
 
diff --git a/spec/models/discourse_single_sign_on_spec.rb b/spec/models/discourse_single_sign_on_spec.rb
index 68c72d1..95f71d0 100644
--- a/spec/models/discourse_single_sign_on_spec.rb
+++ b/spec/models/discourse_single_sign_on_spec.rb
@@ -8,7 +8,7 @@ describe DiscourseSingleSignOn do
     SiteSetting.sso_url = @sso_url
     SiteSetting.enable_sso = true
     SiteSetting.sso_secret = @sso_secret
-    SiteSetting.queue_jobs = false
+    run_jobs_synchronously!
   end
 
   def make_sso
diff --git a/spec/models/post_action_spec.rb b/spec/models/post_action_spec.rb
index 0de0efd..3739283 100644
--- a/spec/models/post_action_spec.rb
+++ b/spec/models/post_action_spec.rb
@@ -1072,7 +1072,7 @@ describe PostAction do
     end
 
     it "should create a notification in the related topic" do
-      SiteSetting.queue_jobs = false
+      run_jobs_synchronously!
       post = Fabricate(:post)
       user = Fabricate(:user)
       action = PostAction.act(user, post, PostActionType.types[:spam], message: "WAT")
@@ -1089,7 +1089,7 @@ describe PostAction do
     end
 
     it "should not add a moderator post when post is flagged via private message" do
-      SiteSetting.queue_jobs = false
+      run_jobs_synchronously!
       post = Fabricate(:post)
       user = Fabricate(:user)

[... diff too long, it was truncated ...]

GitHub sha: d1d9a4f1


Approved #2

#3

If SiteSetting.queue_jobs is confusing, shouldn’t we change the name of the site setting instead? I think we should also avoid using ! here because it is commonly used when a Ruby method mutates state of a object.


#4

I’ve always considered the ! in a Ruby method as “having side effects” and not simply “mutating state of an object”. In that light, the naming of this method makes sense.


#5

This is not a side effect though, I’m not sure how to phrase it properly but setting a site setting is having a “direct” effect since it is being configured by the user who should be aware of the outcome of configuring that setting.


#6

@tgxworld there is a lot of code not in core that uses the site setting (check all-the-plugins for queue_jobs.) It seemed like a lot of work and making a helper like this would allow us to try it and see how it feels before adopting the pain everywhere of a rename.

As for the !, my understanding is it means “not safe”. For example array.chomp is “safe” because if other code has a reference to that object it is not changed. array.chomp! is not safe, because it changes the object itself.

In this case, run_jobs_synchornously! is definitely not safe, as it’s global state that will change how every test runs until it is changed back. Every job will now execute whereas they were skipped before.


#7

What ! represents seem to vary quite abit and I couldn’t find a good guideline. Ruby mainly uses ! for methods that mutate the caller while Rails uses ! In ActiveRecord for methods that raises an error if it fails.

I can try out the method for now but I personally do not think the site setting is confusing. Perhaps the usage of the wrapper method can be made optional?


#8

I find run_jobs_synchronously! simpler to understand in tests cause it conveys intent vs SiteSetting.queue_jobs = false that has intent as a side effect.

That said, odds of me being able to type synchronously without typo approach 0.

Regarding the site setting name I am not against renaming it: (or at least adding an alias) but I struggle to come up with a nicer name.

SiteSetting.run_jobs_immediately

SiteSetting.skip_sidekiq

SiteSetting.run_jobs_inline

SiteSetting.run_jobs_sync

Now that unicorn spawns sidekiq for me in dev I will barely even think of this setting anymore, only in tests


#9

That’s what copy and paste is for :slight_smile:

Yes, this is indeed only relevant in tests at this point.


#10

Maybe the conclusion here is that we should nuke the site setting altogether? Keep it as a helper method on our base Job class?


#11

Yes I think it’s time to get rid of the setting. RIP it lived a good life :ghost:

It’s now based on the Jobs module.