Skip to content

Improve NextInstanceOf stub_new

Qingyu Zhao requested to merge qzhao-improve-next-instance-of into master

What does this MR do and why?

In spec/support/helpers/next_instance_of.rb, when use expect_next_instance_of(klass, *new_args), it currently adds the :new arguments validation check only when new_args.any?.

This is not safe if new_args only contains false/nil values. For example: new_args = [nil], new_args = [false, nil]. For such values, new_args.any? returns false. As a result, we are NOT validating the :new arguments at all.

We can use new_args.present?. [nil].present? returns true. This ensures the stub_new method adds the :new arguments validation as we expected.

Screenshots or screen recordings

These are strongly recommended to assist reviewers and reduce the time to merge your change.

How to set up and validate locally

We can take spec/workers/destroy_pages_deployments_worker_spec.rb to verify this.

  1. Check out master branch
  2. Apply this diff:
diff --git a/spec/workers/destroy_pages_deployments_worker_spec.rb b/spec/workers/destroy_pages_deployments_worker_spec.rb
index 2c20c9004efa..efd074eaafc6 100644
--- a/spec/workers/destroy_pages_deployments_worker_spec.rb
+++ b/spec/workers/destroy_pages_deployments_worker_spec.rb
@@ -17,7 +17,7 @@
   end
 
   it 'can be called without last_deployment_id' do
-    expect_next_instance_of(::Pages::DestroyDeploymentsService, project, nil) do |service|
+    expect_next_instance_of(::Pages::DestroyDeploymentsService, false, false) do |service|
       expect(service).to receive(:execute).and_call_original
     end
  1. run bin/rspec ./spec/workers/destroy_pages_deployments_worker_spec.rb. The spec still passes. This is not expected.
  2. Check out this MR's branch. Repeat above step 2 and step 3, the spec will fail as expected:
Failures:

  1) DestroyPagesDeploymentsWorker can be called without last_deployment_id
     Failure/Error: ::Pages::DestroyDeploymentsService.new(project, last_deployment_id).execute
     
       #<Pages::DestroyDeploymentsService (class)> received :new with unexpected arguments
         expected: (false, false)
              got: (#<Project id:67 namespace3/project3>>, nil)
       Diff:
       @@ -1 +1 @@
       -[false, false]
       +[#<Project id:67 namespace3/project3>>, nil]
       

MR acceptance checklist

This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by Qingyu Zhao

Merge request reports