Move away from RSpec's described_class#22307
Conversation
MikeMcQuaid
left a comment
There was a problem hiding this comment.
Sorry to be that guy: now I'm actually looking at the diff, I kinda hate it 😭.
Could we do something like a type alias or e.g. test_alias = Homebrew::API::Cask::CaskStructGenerator to have RSpec handle this better with Sorbet while avoiding the excessive repetition?
Hah, it is quite a lot of diff.
To avoid the excessive repetition |
Works for me! |
a04db9c to
9292fa6
Compare
66b86b8 to
461521f
Compare
MikeMcQuaid
left a comment
There was a problem hiding this comment.
Looks good when 🟢! Don't need another review, just self-merge whenever no conflicts. Another option: do a partial merge with conflicting files removed and address conflicts in another batch to make it easier to get this landed.
This comment has been minimized.
This comment has been minimized.
- The "explicit" setting on the `RSpec/DescribedClass` cop is still
enabled, because `described_class` is bad for Sorbet typing, but
instead of applying its autocorrections to specify the full class name
everywhere that `described_class` was used, use
`let(:klass) { Foo }` so that the repetition per-test isn't massive in
tests that use it a lot (say, >5 times).
- Changes made with Copilot CLI, agent mode, Claude Opus 4.7.
- Keep specs compatible with stricter runtime checks after replacing `described_class` in typed tests. - Avoid nil secrets and nil process status values tripping Sorbet runtime while tests exercise unauthenticated API and missing command paths.
461521f to
18d51e7
Compare
|
Sorry, had this on my list to get back to, but then 😅🔥 - thanks for finishing it off! |
- After #22307, I looked into what was needed to move the rest of the tests to Sorbet `typed: true` or higher. - Running `spoom` in dry-run mode found that several files only had one error: using the new `klass` or `subject` variable as a dynamic constant, instead of the full constant name (that is, https://sorbet.org/docs/error-reference#5001). - So, I got OpenAI Codex 5.3 Medium to bump all the specs to `typed: true`, search for only 5001 errors. There were 120 errors. Then for each of them I replaced `klass` or `subject` with the full constant name. Now a few more tests are typechecked to `true`, and we have less far to go on the others.
brew lgtm(style, typechecking and tests) with your changes locally?When we moved away from using the RuboCop autocorrections for
described_classreplacements with class names everywhere, I used Copilot with Claude Opus 4.7 in Agent mode to make the changes in commit adf7123 to addlet(:klass) { Foo }and replacedescribed_classwithklassin all the places.