Include thread-safety rubocop and refactor minimum rubocop.gemfile

[alexpark20]

• closes Stop disabling Style/MutableConstant #349
• closes https://github.com/Shopify/storefront-renderer/issues/34154

This PR contains mainly these two changes:

---

Introduce thread_safety rubocop

Include thread_safety rubocop as part of Shopify style rubocop. This PR only enables the small subset of the rules as a starting point. In addition, not part of thread_safety rubocop, but also enabled Style/MutableConstant to prevent constant being mutated concurrently unintentionally.

Note that this is part of Falcon on SFR effort (vault project here).

---

Refactor minimum_rubocop.gemfile

We have updated minimum_rubocop.gemfile so that we could handle the version dependency in tests better .

[rafaelfranca]

This should not be enabled

[alexpark20]

Right, we are not enabling it on SFR too. I will disable it 👍

[rafaelfranca]

This cop is disallowing:

class FooTest < ActiveSupport::TestCase
  setup do
    @something = { a: "A" }
  end
end

Which doesn't have any thread safety issue.

We need to fix that in order to enable it.

[alexpark20]

I will not enable it yet. Good check, lets socialize how we can fix it 👍

[sambostock]

The unknown cop issue is fixed, but now we have the issue that rubocop-thread_safety imposes a minimum version requirement for rubocop which is higher than our current minimum (which we test against, so it breaks CI because the dependencies are unsatisfiable).

[sambostock]

Simply bumping it doesn't work either, as it breaks some assumptions made by the tooling that ensures version compatibility. We'll have to fix that too.

[viralpraxis]

The unknown cop issue is fixed, but now we have the issue that rubocop-thread_safety imposes a minimum version requirement for rubocop which is higher than our current minimum (which we test against, so it breaks CI because the dependencies are unsatisfiable).

Would backporting the patch to 0.6 help? rubocop-thread_safety-0.6 requires rubocop >= 1.48.1

[alexpark20]

The unknown cop issue is fixed, but now we have the issue that rubocop-thread_safety imposes a minimum version requirement for rubocop which is higher than our current minimum (which we test against, so it breaks CI because the dependencies are unsatisfiable).

Would backporting the patch to 0.6 help? rubocop-thread_safety-0.6 requires rubocop >= 1.48.1

We want to have the fix in v0.7.2 😞

Simply bumping it doesn't work either, as it breaks some assumptions made by the tooling that ensures version compatibility. We'll have to fix that too.

What are those assumptions?

[viralpraxis]

We want to have the fix in v0.7.2 😞

Sorry, I didnt quite get it since I don't have all the context. The patch was released in v0.7.1 and is also present in v0.7.2 (which was released today to fix an unreleated issue).

[sambostock]

Both options work. The issue in CI is that we have a check that is along the lines of

Extract the Rubocop version from the gemspec, and run CI against that version.

The check currently extracts ~> x.y and runs CI with Rubocop version x.y, but that breaks because rubocop-thread_safety depends on ~> 1.72, >= 1.72.1, so if we try to just depend on ~> 1.72, CI fails because = 1.72 is incompatible with >= 1.72.1.

The backport idea would be nice because it would mean we don't have to increase the dependency requirement, but realistically that's not a blocker. I just have to fix CI to be smart enough to handle this case.

[alexpark20]

@sambostock, I updated minimum_rubocop.gemfile, LMK what you think :)

[sambostock]

We don't want to use ~>, because that doesn't guarantee that we're using the exact minimum version. For example, this results in ~> 1.72.0 which would likely result in us installing 1.72.2, whereas the actual minimum version we should be testing against is 1.71.1.

[sambostock]

I've switched to an approach of extracting the rubocop requirements from the gemspec, ensuring they're all using ~> or >=, and then grabbing the max version and using that as the exact version.

The other alternative I thought of was to get all the published versions from Rubygems, and then Array#bsearch through them and use satisfied_by? to find the first one that satisfies our requirements (and maybe sub-dependency requirements too), and use that. However, that approach would be more complex, and the other approach is probably sufficient.

[alexpark20]

Agreed, extracting ~> and >= and grab the max version makes it more straightforward 👍

[sambostock]

We should rebase/squash before merging if we're okay with these rules and the change to minimum_rubocop.gemfile.

[alexpark20]

Clever 👏