`stub_licensed_features` does not work sometimes due to memoization

Because Project#licensed_feature_available? is strong_memoized, sometimes stub_licensed_features does not do anything because License.feature_available?(feature) will no longer be called.

See the following discussion from !9970 (merged) where this happens:

• @tkuah started a discussion: (+5 comments)

  Does stub_licensed_features(file_locks: false) not work here ?

Workaround

The current workaround is to clear the memoization before each example:

h/t @splattael for this (#10267 (comment 148344480)):

  before do
    # We use `let_it_be(:project)` so we make sure to clear caches
    project.clear_memoization(:licensed_feature_available)
  end

added Category:License Compliance [DEPRECATED] backstage [DEPRECATED] test labels

changed the description

changed title from stub_licensed_features does not work sometimes due to memoization to {+stub_licensed_features+} does not work sometimes due to memoization

unassigned @tkuah

This is related to https://gitlab.com/gitlab-org/gitlab-ee/issues/10267 but here no set was involved.

I think for this one it's probably because let!(:lock) which runs before stub_licensed_features. We can maybe fix it by using just let, but then we'll also need to update before which does project.add_developer(current_user).

I am not sure if there can be a better fix than "not memoizing" that in the tests. If we can have a very_first_before that can help.

mentioned in issue #10267 (closed)

Gotcha, I assumed it was a factory, but you are right. Should be an easy fix then, for that spec.

mentioned in merge request !9994 (merged)

Setting devopssecure ~"group::software composition analysis" based on ~"license management".

Setting devopssecure ~"group::software composition analysis" based on ~"license management".

added devopssecure groupcomposition analysis labels

added devopssecure groupcomposition analysis labels

added [deprecated] Accepting merge requests label

removed [deprecated] Accepting merge requests label

added Enterprise Edition label

marked this issue as related to #10267 (closed)

mentioned in merge request !15160 (merged)

added backend label

added to epic &1662 (closed)

Although very desirable the overall ~"Category:License Compliance" work is lower priority than other asks at this time and will be delayed. See gitlab-org/secure/general#65 (closed) for more information.

changed milestone to %Backlog

added [deprecated] Accepting merge requests label

changed milestone to %13.2

can someone check if a) still an issue b) come up with plan to fix so i can see how heavy/large/complex this work is?

@NicoleSchwartz I don't think this is related to ~"Category:License Compliance". This issue is related to how we stub GitLab licensed features and not software licenses associated with detected dependencies.

@xlgmokha you're right. I blindly applied the refinement label on this but it's clearly outside our product area.

@tkuah or @godfat should this belong to Quality dept?

Yea, I think so /cc @rymai

added workflowplanning breakdown label

added secure:refinement-backend label

removed Category:License Compliance [DEPRECATED] Enterprise Edition groupcomposition analysis secure:refinement-backend workflowplanning breakdown labels

removed from epic &1662 (closed)

removed devopssecure label

added Quality label

added Engineering Productivity maintenancerefactor labels

changed milestone to %Backlog

added typemaintenance label and removed backstage [DEPRECATED] label

added typefeature label

changed the description

@tkuah I've added a the workaround into the description. While not perfect, it's possibly always faster than avoiding use of let_it_be.

Thanks @seanarnold for flagging me here!

I think with let_it_be we can also use let_it_be_with_refind (let_it_be_with_reload won't work):

  describe 'memoization' do
    let_it_be_with_refind(:project) { create(:project) }

    subject { project.feature_available?(:epics) }

    context 'license available' do
      before do
        stub_licensed_features(epics: true)
      end

      it { is_expected.to eq true }
    end

    context 'license not available' do
      it { is_expected.to eq false }
    end
  end

Oh, interesting! Clearing the memoization might be faster still since it doesn't touch the DB?

@seanarnold Good point, using let_it_be_with_refind might be slower but I'd love to see how much slower

Personally, I prefer a slightly slower specs over the usage of internal APIs which are clearing some internal variables.

For example, in !41144 (merged) we are actually removing this internal API from CORE specs because they don't have any effect.

mentioned in merge request !41144 (merged)

mentioned in merge request !44253 (merged)

mentioned in merge request !44546 (merged)

mentioned in merge request !49791 (merged)

mentioned in merge request !46731 (merged)

mentioned in merge request !51290 (merged)

Just burned a few hours on this Both workarounds (clear_memoization and let_it_be_with_refind) fixed this I'll probably go with let_it_be_with_refind for now.

@nfriend Glad you found a solution

Note that both solutions has been documented lately.

@tkuah I wonder if we could even close this issue then?

Edit: Fixed the link

Note that both solutions has been documented lately.

Nice, I missed that!

I wonder if we could even close this issue then?

It's still an issue, and I think we want to find a automatic way eventually.

@tkuah Oh I see

The word "automatic" is the key here

OK, I've taken a stab at it with !54215 (diffs) by patching Project#licensed_feature_available? in specs

Since !54215 (merged) has been merged we no longer need to worry about clearing projects' license feature caches in specs. It's now done transparently.

Announcing in Engineering-in-Review and closing.

Thanks @splattael !

Awesome! Thanks @splattael! I think this will save a lot of future time and frustration

@splattael I just ran into this issue again, but with a group. WDYT of doing the same thing you did in !54215 (merged) but for groups? (Patching Group#licensed_feature_available?)

cc @tkuah

@nfriend Please open a new issue :)

@tkuah Good call Done: #335054 (closed)

mentioned in merge request !54215 (merged)

closed

mentioned in issue #335054 (closed)

removed typefeature label

With !54215 (merged) we bypass memoization for licensed_feature_available. The spec helper with_license_feature_cache be used if relying on feature cache is required.

Closing

changed milestone to %13.10