DEV Community

Pavan Kumar Sadashiv
Pavan Kumar Sadashiv

Posted on Edited on

Part 5: Catching Vacuous RSpec Mocks & Adapting to Mid-Flight Feature Flag Cleanups (SecretsHelperSpec)

#gitlab #testing #rspec #ruby

Series Context: Part 5 of 10 in GitLab Core Engineering: Architecture, MCP & CI/CD Deep Dives.

Featured MR: gitlab-org/gitlab!269061 · Work Item: #635243

What Makes a Spec "Vacuous"?

A vacuous (tautological) test passes not because your production code behaves correctly, but because your test harness already short-circuited the execution path under assertion. Worse than having no test at all, a vacuous negative mock (expect(Service).not_to receive(:call)) creates false security during future refactors—someone could introduce an expensive network lookup inside the real policy layer and this test would still stay green.

In ee/spec/helpers/secrets_helper_spec.rb, we found an instructive example of this exact anti-pattern—and then watched our MR shrink from a multi-file feature-flag cleanup into a surgical 6-line deletion after master moved underneath us.

Dissecting the Vacuous Mock in SecretsHelperSpec

Look at the enclosing before block for #secrets_manager_variables_cta_data in ee/spec/helpers/secrets_helper_spec.rb:

before do
  stub_feature_flags(secrets_manager_variables_cta: true)
  allow(helper).to receive_messages(current_user: current_user, can?: false)
  allow(helper).to receive(:can?).with(current_user, :start_secrets_manager_trial)
    .and_return(can_start_instance_trial)
  allow(helper).to receive(:can?).with(current_user, :start_secrets_manager_trial, cta_group)
    .and_return(can_start_group_trial)
  allow(helper).to receive(:can?).with(current_user, :read_project_secrets, anything)
    .and_return(can_read_project_secrets)
  allow(helper).to receive(:can?).with(current_user, :read_secret, anything).and_return(can_read_group_secrets)
  ...
end
Enter fullscreen mode Exit fullscreen mode

Notice what allow(helper).to receive(:can?) does: real DeclarativePolicy evaluation is completely bypassed. Every can? call inside SecretsHelper#secrets_manager_cta_data returns a canned RSpec stub boolean without ever instantiating a Policy object or touching SecretsManagement::Entitlement.

Yet deeper inside a nested context (context 'when the group has never enrolled' -> context 'with a project and a user who can start a trial'), there sat this example:

it 'makes no CustomersDot lookup' do
  expect(SecretsManagement::Entitlement).not_to receive(:for)

  data
end
Enter fullscreen mode Exit fullscreen mode

Why was that test 100% vacuous?

  1. SecretsHelper#secrets_manager_cta_data itself never references SecretsManagement::Entitlement—any entitlement/CustomersDot lookup could only occur inside real policy rules evaluated by can?.
  2. Because helper.can? was stubbed in the enclosing before block, the policy engine is never invoked at all when data runs!
  3. Removing SecretsManagement::Entitlement.for calls or adding five of them inside the real policy or availability classes would still pass this example every single time.

The fix in !269061 is clean deletion:

--- a/ee/spec/helpers/secrets_helper_spec.rb
+++ b/ee/spec/helpers/secrets_helper_spec.rb
@@ -684,12 +684,6 @@
             secrets_path: project_secrets_path(cta_project), start_trial_path: group_secrets_path(cta_group)
           )
         end
-
-        it 'makes no CustomersDot lookup' do
-          expect(SecretsManagement::Entitlement).not_to receive(:for)
-
-          data
-        end
       end
Enter fullscreen mode Exit fullscreen mode

When upstream/master Ships Half Your MR While You Rebase

Here is a practical GitLab contribution scenario you will inevitably hit: !269061 originally paired removing the secrets_manager_variables_cta feature flag / partial with deleting the vacuous spec. While our branch was queued for review, upstream commit 5350f0e843 (Clean up secrets_manager_variables_cta feature flag) landed on master, removing the flag and _nudge_secrets_manager.html.haml* views.

When rebasing cleanly onto upstream/master (commit 745ca762b3):

  1. All feature-flag and Haml view changes vanished automatically via 3-way merge because upstream already landed them.
  2. Only the untouched 6-line spec removal in ee/spec/helpers/secrets_helper_spec.rb survived.

Don't stop at git push --force-with-lease! Whenever a rebase dramatically narrows an MR's scope, you have three metadata hygiene duties in GitLab:

  1. Rewrite the MR title & description: Leaving "Clean up feature flag and remove vacuous spec" on a 6-line spec-only diff wastes reviewer time. We updated !269061's title and description to match reality: Remove vacuous CustomersDot lookup spec in SecretsHelperSpec.
  2. Fix Commit Trailers (EE: true): Even though the change is only a test deletion, it lives under ee/spec/. Danger immediately flags missing EE: true trailers on EE-only commits unless present.
  3. Update Labels & Trigger pipeline:spec-only: Because no production code (app/, lib/, Haml) remained in the diff, stale ~"frontend" / ~UX labels misled Roulette, whereas ~"pipeline:spec-only" (checked by danger/spec_only/Dangerfile) skips irrelevant heavy build/asset jobs and runs focused specs fast. Our rebased pipeline #3053445498 passed 56 jobs green!

Takeaways for GitLab Engineers

  1. Audit negative mock assertions (not_to receive): Whenever you write expect(Foo).not_to receive(:bar), ask: "Did an enclosing before block stub out the caller that would actually invoke Foo.bar?" If yes, either unstub the boundary (test with real objects) or test at the policy layer where Foo.bar actually lives.
  2. Keep MR titles, descriptions, and labels in sync after rebases: When master absorbs part of your MR, scrub outdated copy and ask a reviewer/maintainer (@zcuddy here) to adjust workflow/pipeline labels (~"pipeline:spec-only").

Series Navigation

Top comments (0)