Only run pending migrations check if necessary

[jeffcarbs]

Motivation

A point of friction we've experienced is getting blocked from running DSL generation due to pending migration even when we're not generating RBI for ActiveRecord models (e.g. when doing something like bin/tapioca dsl Some::Sidekiq::Worker Some::Graphql::Mutation) . The pending migration check is valuable to ensure we don't generate ActiveRecord RBI with a stale schema, but it's irrelevant when generating RBI for things like sidekiq workers, graphql mutations, or other non-ActiveRecord things.

Implementation

Since we want to conditionalize running the pending migrations check, we need to be at a point where we have the constants we're going to process in memory. This means we need to pull the pending migrations check out from the Loaders::Dsl further down into the Dsl::Pipeline. We then check to see if any of the constants we're going to process subclass ActiveRecord::Base and only check for pending migrations then.

This handles both the case where you don't give specific classes (bin/tapioca dsl) or you give some (bin/tapioca dsl Something) and there are test cases for both scenarios.

A small side-benefit here is that previously even a command like bin/tapiocs dsl --list-compilers would fail due to pending migrations since that check happened when loading rails. Since it now only runs if necessary, that command would succeed.

Tests

I refactored the existing pending migration test to reuse some pieces (see first commit) and then added some specific test cases for this.

[jeffcarbs]

I intentionally dropped this line when moving this helper method since it would require us to thread app_root further down into Dsl::Pipeline. It was added in the original PR that added this check (89de7fd#diff-3c285599de82036518463c68957ff3a1b87239257be68b33041c5b9bef3dba1aR700) but I can't really tell what it's trying to do. My best guess is that it's trying to ensure we're inside a Rails app and since this check is now conditionalized on ActiveRecord::Base being defined I think we can rely on that for the same assumption check.

[vinistock]

Yeah, the idea was to check if we were in a Rails app before it was loaded. I agree that we probably won't need it if we can check for ActiveRecord::Base.

[vinistock]

Yeah, the idea was to check if we were in a Rails app before it was loaded. I agree that we probably won't need it if we can check for ActiveRecord::Base.

[vinistock]

Same here.

[vinistock]

I think you may not even need this one now that we're further down the path.

[jeffcarbs]

I've removed this line as well 👍

[jeffcarbs]

Had to put this back since it broke some tests (e.g. https://github.com/Shopify/tapioca/actions/runs/4317148630/jobs/7533849593#step:6:410). Couldn't hurt to be a little cautious here.

[jeffcarbs]

cc @paracycle @vinistock - I think there's something wrong with the test suite. Some unrelated specs for it must do mixin attribution properly were failing on this PR so I was trying to investigate. I couldn't repro locally so I thought maybe it was some sort of test pollution but after trying various permutations I wound up just reverting all my changes so this PR is literally just adding a comment and the rails 6-1 specs are still failing: https://github.com/jeffcarbs/tapioca/actions/runs/4287122212/jobs/7467586895#step:6:168

[dirceu]

@jeffcarbs thanks for reporting! I believe we found the problem; I'll ping this PR to let you know to rebase once we've fixed it.

[dirceu]

@jeffcarbs sorry about the delay; the CI issue has been fixed. Can you push your changes again?

[jeffcarbs]

Thanks @dirceu, looks like it's working now!

@vinistock - I've put the changes back and the build is green so this should be good to go

[paracycle]

I have a small comment about safety in the presence of arbitrary constants, otherwise, this is great work. Thanks for the PR!