Skip to content
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

Improve Active Job test helpers #33635

Merged
merged 9 commits into from Aug 20, 2018

Conversation

Projects
None yet
5 participants
@bogdanvlviv
Copy link
Contributor

bogdanvlviv commented Aug 16, 2018

  • 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 #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 #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 #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

jeremy approved these changes Aug 17, 2018

Copy link
Member

jeremy left a comment

Fantastic work @bogdanvlviv.

activejob/CHANGELOG.md Outdated
```

*bogdanvlviv*

This comment has been minimized.

@jeremy

jeremy Aug 17, 2018

Member

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

This comment has been minimized.

@bogdanvlviv

bogdanvlviv Aug 20, 2018

Author Contributor

Done in c6f4ddf.

activejob/CHANGELOG.md Outdated
@@ -11,7 +65,7 @@

*Zach Kemp*

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

This comment has been minimized.

@jeremy
activejob/lib/active_job/queue_adapters/test_adapter.rb Outdated
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

This comment has been minimized.

@jeremy

jeremy Aug 17, 2018

Member

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

This comment has been minimized.

@bogdanvlviv

bogdanvlviv Aug 20, 2018

Author Contributor

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

@@ -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.

This comment has been minimized.

@jeremy

jeremy Aug 17, 2018

Member

💅 love these edits

This comment has been minimized.

@MarcSteven
activejob/lib/active_job/test_helper.rb Outdated
end
assert_equal number, actual_count, "#{number} jobs expected, but #{actual_count} were enqueued"

This comment has been minimized.

@jeremy

jeremy Aug 17, 2018

Member

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

Show resolved Hide resolved activejob/lib/active_job/test_helper.rb
#
# perform_enqueued_jobs
#
# assert_performed_jobs 2

This comment has been minimized.

@jeremy

jeremy Aug 17, 2018

Member

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

This comment has been minimized.

@bogdanvlviv

bogdanvlviv Aug 20, 2018

Author Contributor

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?

This comment has been minimized.

@jeremy

jeremy Aug 20, 2018

Member

Agreed 👍

@jeremy

This comment has been minimized.

Copy link
Member

jeremy commented Aug 17, 2018

@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.)

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 #33265

[bogdanvlviv & Jeremy Daer]

@bogdanvlviv bogdanvlviv force-pushed the bogdanvlviv:improve-active_job-test_helpers branch Aug 20, 2018

@bogdanvlviv

This comment has been minimized.

Copy link
Contributor Author

bogdanvlviv commented Aug 20, 2018

@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

This comment has been minimized.

Copy link
Member

kamipo commented Aug 20, 2018

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

This comment has been minimized.

Copy link
Contributor Author

bogdanvlviv commented Aug 20, 2018

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

This comment has been minimized.

Copy link
Member

kamipo commented Aug 20, 2018

enqueued_jobs_with in flush_enqueued_jobs also does validate_option in the first line.

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).

bogdanvlviv added some commits Aug 16, 2018

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.
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
```
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
```
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.
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 #33626.
Add changelog entry about adding `:queue` option to job assertions an…
…d helpers

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

@bogdanvlviv bogdanvlviv force-pushed the bogdanvlviv:improve-active_job-test_helpers branch to b857642 Aug 20, 2018

@bogdanvlviv

This comment has been minimized.

Copy link
Contributor Author

bogdanvlviv commented Aug 20, 2018

#33635 (comment)

@kamipo Good point. 👍

Fixed in e0cf042.

@jeremy

jeremy approved these changes Aug 20, 2018

Copy link
Member

jeremy left a comment

👏

@Edouard-chin

This comment has been minimized.

Copy link
Contributor

Edouard-chin commented Aug 20, 2018

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

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

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@bogdanvlviv bogdanvlviv deleted the bogdanvlviv:improve-active_job-test_helpers branch Aug 21, 2018

@bogdanvlviv

This comment has been minimized.

Copy link
Contributor Author

bogdanvlviv commented Aug 21, 2018

Thanks all 🙇 !

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

Remove duplicate test
This patch corrects a duplicate method name introduced in rails#33635.

Also fixes typo in method names.

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

Remove duplicate test
This patch corrects a duplicate method name introduced in #33635.

Also fixes typo in method names.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.