Improve Active Job test helpers #33635

bogdanvlviv · 2018-08-16T20:07:11Z

ec2e8f6

Allow :queue option to perform_enqueued_jobs.

If the :queue option is specified, then only the job(s) enqueued to
a specific queue will be performed.

Example:

def test_perform_enqueued_jobs_with_queue
  perform_enqueued_jobs queue: :some_queue do
    MyJob.set(queue: :some_queue).perform_later(1, 2, 3) # will be performed
    HelloJob.set(queue: :other_queue).perform_later(1, 2, 3) # will not be performed
  end
  assert_performed_jobs 1
end

Follow up Allow queue option to assert_no_enqueued_jobs #33265

[bogdanvlviv & Jeremy Daer]

e0cf042

Fix perform_enqueued_jobs

Set

queue_adapter.perform_enqueued_jobs = true
queue_adapter.perform_enqueued_at_jobs = true
queue_adapter.filter = only
queue_adapter.reject = except
queue_adapter.queue = queue
```
if block given.
Execution of `flush_enqueued_jobs` doesn't require that.

d50fb21

Allow :queue option to assert_performed_jobs.

If the :queue option is specified, then only the job(s) enqueued to a specific
queue will be performed.

Example:

def test_assert_performed_jobs_with_queue_option
  assert_performed_jobs 1, queue: :some_queue do
    HelloJob.set(queue: :some_queue).perform_later("jeremy")
    HelloJob.set(queue: :other_queue).perform_later("bogdan")
  end
end

de4420d

Allow :queue option to assert_no_performed_jobs.

If the :queue option is specified, then only the job(s) enqueued to a specific
queue will not be performed.

Example:

def test_assert_no_performed_jobs_with_queue_option
  assert_no_performed_jobs queue: :some_queue do
    HelloJob.set(queue: :other_queue).perform_later("jeremy")
  end
end

2bf8b4e

Add changelog entry about adding :queue option to job assertions and helpers

Note that it removes changelog entry of Allow queue option to assert_no_enqueued_jobs #33265 since the entry in this commits
includes that too.

11634e8

Fix assert_performed_jobs and assert_no_performed_jobs

Execution of assert_performed_jobs, and assert_no_performed_jobs
without a block should respect passed :except, :only, and :queue options.

2ec60fb

Allow assert_performed_with to be called without a block.

Example:

def test_assert_performed_with
  MyJob.perform_later(1,2,3)

  perform_enqueued_jobs

  assert_performed_with(job: MyJob, args: [1,2,3], queue: 'high')
end

Follow up Allow perform_enqueued_jobs to be called without a block. #33626.

b7beb5d

Fix formatting of ActiveJob::TestHelper api docs

b857642

DRY in assert_enqueued_jobs

Note that these changes keep backward compatibility, so It shouldn't break users' tests during an upgrade to 6.0.

jeremy

Fantastic work @bogdanvlviv.

jeremy · 2018-08-17T17:33:51Z

activejob/CHANGELOG.md

+    ```
+
+    *bogdanvlviv*
+


Let's combine into one changelog entry that heralds these together, that :queue may be used with job assertions & helpers.

Done in c6f4ddf.

jeremy · 2018-08-17T17:34:13Z

activejob/CHANGELOG.md

@@ -11,7 +65,7 @@

    *Zach Kemp*

-*   Allow `queue` option to `assert_no_enqueued_jobs`.
+*   Allow `:queue` option to `assert_no_enqueued_jobs`.


jeremy · 2018-08-17T17:40:21Z

activejob/lib/active_job/queue_adapters/test_adapter.rb

-            Array(reject).include?(job.class)
+            Array(reject).include?(job.class) || (queue && job.queue_name != queue.to_s)
+          elsif queue
+            job.queue_name != queue.to_s
          else
            false
          end


Can hoist up the queue conditional since it applies to every clause here:

def filtered?(job) filtered_queue?(job) || filtered_job_class?(job) end def filtered_queue?(job) if queue job.queue_name != queue.to_s end end def filtered_job_class?(job) if filter !Array(filter).include?(job.class) elsif reject Array(reject).include?(job.class) end end

😍 Thanks for the tip. I like this simplified format. I applied this suggestion in ec2e8f6 and added you to authors of this commit.

jeremy · 2018-08-17T17:41:00Z

activejob/lib/active_job/test_helper.rb

@@ -52,7 +52,7 @@ def after_teardown # :nodoc:
      queue_adapter_changed_jobs.each { |klass| klass.disable_test_adapter }
    end

-    # Specifies the queue adapter to use with all active job test helpers.
+    # Specifies the queue adapter to use with all Active Job test helpers.


💅 love these edits

jeremy · 2018-08-17T17:41:51Z

activejob/lib/active_job/test_helper.rb

      end
+      assert_equal number, actual_count, "#{number} jobs expected, but #{actual_count} were enqueued"


Nice style change. To confirm, I'm not seeing any implementation change here?

activejob/lib/active_job/test_helper.rb

jeremy · 2018-08-17T17:43:27Z

activejob/lib/active_job/test_helper.rb

+    #
+    #     perform_enqueued_jobs
+    #
+    #     assert_performed_jobs 2


We're showing two API calling styles here. Let's provide some guidance about when you'd use one vs. the other.

Hm. Then we should provide guidance about the one vs the other to every helper/assertion.
I believe that this difference is explicit from the current explanations like
"If a block is passed, that block performs all of the jobs that were enqueued throughout the duration of the block and asserts that the job has been performed with the given arguments in the block." and examples.
Let's go with as it is for now?

Agreed 👍

jeremy · 2018-08-17T17:49:32Z

@bogdanvlviv Do you think 2b471e1 is safe to backport to 5-2-stable without breaking compatibility? (As far as I can see, it's a bugfix.)

If the `:queue` option is specified, then only the job(s) enqueued to a specific queue will be performed. Example: ``` def test_perform_enqueued_jobs_with_queue perform_enqueued_jobs queue: :some_queue do MyJob.set(queue: :some_queue).perform_later(1, 2, 3) # will be performed HelloJob.set(queue: :other_queue).perform_later(1, 2, 3) # will not be performed end assert_performed_jobs 1 end ``` Follow up rails#33265 [bogdanvlviv & Jeremy Daer]

bogdanvlviv · 2018-08-20T08:53:34Z

@bogdanvlviv Do you think 2b471e1 is safe to backport to 5-2-stable without breaking compatibility? (As far as I can see, it's a bugfix.)

@jeremy We shouldn't backport e0cf042 to 5-2-stable since execution of perform_enqueued_jobs without a block was allowed only on the master branch in #33626.

The current implementation of perform_enqueued_jobs on 5-2-stable: https://github.com/rails/rails/blob/5-2-stable/activejob/lib/active_job/test_helper.rb#L368L368-L386

kamipo · 2018-08-20T09:08:23Z

How about an alternative of b43d3c07b5eeff5c0569b901933d17071e387b36 to touch few code like here?

diff --git a/activejob/lib/active_job/test_helper.rb b/activejob/lib/active_job/test_helper.rb
index b45cc57fb0..b130b9604b 100644
--- a/activejob/lib/active_job/test_helper.rb
+++ b/activejob/lib/active_job/test_helper.rb
@@ -404,6 +404,8 @@ def assert_performed_with(job: nil, args: nil, at: nil, queue: nil)
     #   end
     #
     def perform_enqueued_jobs(only: nil, except: nil)
+      return flush_enqueued_jobs(only: only, except: except) unless block_given?
+
       validate_option(only: only, except: except)
       old_perform_enqueued_jobs = queue_adapter.perform_enqueued_jobs
       old_perform_enqueued_at_jobs = queue_adapter.perform_enqueued_at_jobs
@@ -416,7 +418,7 @@ def perform_enqueued_jobs(only: nil, except: nil)
         queue_adapter.filter = only
         queue_adapter.reject = except
 
-        block_given? ? yield : flush_enqueued_jobs(only: only, except: except)
+        yield
       ensure
         queue_adapter.perform_enqueued_jobs = old_perform_enqueued_jobs
         queue_adapter.perform_enqueued_at_jobs = old_perform_enqueued_at_jobs

bogdanvlviv · 2018-08-20T09:16:50Z

I want to move

old_perform_enqueued_jobs = queue_adapter.perform_enqueued_jobs
old_perform_enqueued_at_jobs = queue_adapter.perform_enqueued_at_jobs
old_filter = queue_adapter.filter
old_reject = queue_adapter.reject
old_queue = queue_adapter.queue

to begin ... end too.

Also we should execute flush_enqueued_jobs after validate_option.

Do you want me to redo b43d3c07b5eeff5c0569b901933d17071e387b36?

kamipo · 2018-08-20T09:33:24Z

enqueued_jobs_with in flush_enqueued_jobs also does validate_option in the first line.

rails/activejob/lib/active_job/test_helper.rb

Lines 446 to 447 in 265eeb2

    
           def enqueued_jobs_with(only: nil, except: nil, queue: nil) 
        
             validate_option(only: only, except: except)

Is there any reason to do validate_option twice?

Personally I feel the remaining part in the commit is like a cosmetic (moving a code chunk, changing indentation, for the purpose of the commit).

Set ```` queue_adapter.perform_enqueued_jobs = true queue_adapter.perform_enqueued_at_jobs = true queue_adapter.filter = only queue_adapter.reject = except queue_adapter.queue = queue ``` if block given. Execution of `flush_enqueued_jobs` doesn't require that.

If the `:queue` option is specified, then only the job(s) enqueued to a specific queue will be performed. Example: ``` def test_assert_performed_jobs_with_queue_option assert_performed_jobs 1, queue: :some_queue do HelloJob.set(queue: :some_queue).perform_later("jeremy") HelloJob.set(queue: :other_queue).perform_later("bogdan") end end ```

If the `:queue` option is specified, then only the job(s) enqueued to a specific queue will not be performed. Example: ``` def test_assert_no_performed_jobs_with_queue_option assert_no_performed_jobs queue: :some_queue do HelloJob.set(queue: :other_queue).perform_later("jeremy") end end ```

…d helpers Note that it removes changelog entry of rails#33265 since the entry in this commits includes that too.

Execution of `assert_performed_jobs`, and `assert_no_performed_jobs` without a block should respect passed `:except`, `:only`, and `:queue` options.

Example: ``` def test_assert_performed_with MyJob.perform_later(1,2,3) perform_enqueued_jobs assert_performed_with(job: MyJob, args: [1,2,3], queue: 'high') end ``` Follow up rails#33626.

bogdanvlviv · 2018-08-20T10:08:12Z

#33635 (comment)

@kamipo Good point. 👍

Fixed in e0cf042.

jeremy

👏

Edouard-chin · 2018-08-20T20:20:47Z

Those are really great addition to AJ test helper, thanks ❤️ !

bogdanvlviv · 2018-08-21T07:09:55Z

Thanks all 🙇‍♂️ !

This patch corrects a duplicate method name introduced in rails#33635. Also fixes typo in method names.

This patch corrects a duplicate method name introduced in #33635. Also fixes typo in method names.

jeremy approved these changes Aug 17, 2018

View reviewed changes

bogdanvlviv force-pushed the improve-active_job-test_helpers branch from 20555dc to ebb637c Compare August 20, 2018 08:32

bogdanvlviv added 8 commits August 20, 2018 13:05

Fix perform_enqueued_jobs

e0cf042

Set ```` queue_adapter.perform_enqueued_jobs = true queue_adapter.perform_enqueued_at_jobs = true queue_adapter.filter = only queue_adapter.reject = except queue_adapter.queue = queue ``` if block given. Execution of `flush_enqueued_jobs` doesn't require that.

Add changelog entry about adding :queue option to job assertions an…

2bf8b4e

…d helpers Note that it removes changelog entry of rails#33265 since the entry in this commits includes that too.

Fix assert_performed_jobs and assert_no_performed_jobs

11634e8

Execution of `assert_performed_jobs`, and `assert_no_performed_jobs` without a block should respect passed `:except`, `:only`, and `:queue` options.

Allow assert_performed_with to be called without a block.

2ec60fb

Example: ``` def test_assert_performed_with MyJob.perform_later(1,2,3) perform_enqueued_jobs assert_performed_with(job: MyJob, args: [1,2,3], queue: 'high') end ``` Follow up rails#33626.

Fix formatting of ActiveJob::TestHelper api docs

b7beb5d

DRY in assert_enqueued_jobs

b857642

bogdanvlviv force-pushed the improve-active_job-test_helpers branch from ebb637c to b857642 Compare August 20, 2018 10:06

jeremy approved these changes Aug 20, 2018

View reviewed changes

jeremy merged commit 2b594b0 into rails:master Aug 20, 2018

bogdanvlviv deleted the improve-active_job-test_helpers branch August 21, 2018 07:07

utilum mentioned this pull request Aug 22, 2018

Fix duplicate test method names #33684

Merged

utilum added a commit to utilum/rails that referenced this pull request Aug 22, 2018

Remove duplicate test

cade790

This patch corrects a duplicate method name introduced in rails#33635. Also fixes typo in method names.

jeremy pushed a commit that referenced this pull request Aug 22, 2018

Remove duplicate test

96ac7e4

This patch corrects a duplicate method name introduced in #33635. Also fixes typo in method names.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Active Job test helpers #33635

Improve Active Job test helpers #33635

bogdanvlviv commented Aug 16, 2018 •

edited

jeremy left a comment

jeremy Aug 17, 2018

bogdanvlviv Aug 20, 2018

jeremy Aug 17, 2018

jeremy Aug 17, 2018

bogdanvlviv Aug 20, 2018

jeremy Aug 17, 2018

MarcSteven Aug 31, 2018

jeremy Aug 17, 2018

jeremy Aug 17, 2018

bogdanvlviv Aug 20, 2018

jeremy Aug 20, 2018

jeremy commented Aug 17, 2018 •

edited

bogdanvlviv commented Aug 20, 2018 •

edited

kamipo commented Aug 20, 2018

bogdanvlviv commented Aug 20, 2018

kamipo commented Aug 20, 2018

bogdanvlviv commented Aug 20, 2018

jeremy left a comment

Edouard-chin commented Aug 20, 2018

bogdanvlviv commented Aug 21, 2018

		end
		assert_equal number, actual_count, "#{number} jobs expected, but #{actual_count} were enqueued"

Improve Active Job test helpers #33635

Improve Active Job test helpers #33635

Conversation

bogdanvlviv commented Aug 16, 2018 • edited

jeremy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeremy commented Aug 17, 2018 • edited

bogdanvlviv commented Aug 20, 2018 • edited

kamipo commented Aug 20, 2018

bogdanvlviv commented Aug 20, 2018

kamipo commented Aug 20, 2018

bogdanvlviv commented Aug 20, 2018

jeremy left a comment

Choose a reason for hiding this comment

Edouard-chin commented Aug 20, 2018

bogdanvlviv commented Aug 21, 2018

bogdanvlviv commented Aug 16, 2018 •

edited

jeremy commented Aug 17, 2018 •

edited

bogdanvlviv commented Aug 20, 2018 •

edited