Skip to content

tests: Use full constant names sometimes (for Sorbet rule 5001 compat)#22415

Merged
MikeMcQuaid merged 1 commit into
mainfrom
even-described-class-alternatives-are-still-bad-for-sorbet-sometimes
May 25, 2026
Merged

tests: Use full constant names sometimes (for Sorbet rule 5001 compat)#22415
MikeMcQuaid merged 1 commit into
mainfrom
even-described-class-alternatives-are-still-bad-for-sorbet-sometimes

Conversation

@issyl0
Copy link
Copy Markdown
Member

@issyl0 issyl0 commented May 25, 2026


  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them? Performance claims (e.g. "this is faster") must include Hyperfine benchmarks.
  • Have you written new tests (excluding integration tests) for your changes? Here's an example.
  • Have you successfully run brew lgtm (style, typechecking and tests) with your changes locally?

  • AI was used to generate or assist with generating this PR.

I got OpenAI Codex 5.3 Medium to bump all the specs to typed: true (because Spoom wouldn't do it automatically), then run brew tc for only code 5001 errors. There were 120 of them.


  • After Move away from RSpec's described_class #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).
  • For each of the 120 errors upon bumping files, I replaced klass or subject with the full constant name. Now 30 more tests are typechecked to true, and we have less far to go on the others.

- 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.
@issyl0 issyl0 marked this pull request as ready for review May 25, 2026 19:57
Copilot AI review requested due to automatic review settings May 25, 2026 19:57
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR reduces Sorbet error 5001 (“dynamic constant reference”) in the test suite to enable more specs to be bumped to # typed: true and successfully typechecked. It follows up on the earlier migration away from described_class by replacing klass::CONST / subject::CONST usages with fully qualified constant names where needed.

Changes:

  • Bump a set of spec files from # typed: false to # typed: true.
  • Replace dynamic constant references in specs with fully qualified constants (e.g., Homebrew::...::CONST, MacOSVersion::NULL, etc.) to satisfy Sorbet 5001.
  • Adjust a small number of expectations/instantiations to avoid dynamic constant usage in test code.
Show a summary per file
File Description
Library/Homebrew/test/version_spec.rb Use Version::NULL_TOKEN instead of a dynamic klass::NULL_TOKEN reference.
Library/Homebrew/test/os/mac/sdk_spec.rb Avoid dynamic exception constant reference when asserting SDK lookup failures.
Library/Homebrew/test/os/mac/reinstall_spec.rb Bump to typed: true and use fully qualified Homebrew::Reinstall::InstallationContext.
Library/Homebrew/test/os/linux/libstdcxx_spec.rb Use fully qualified OS::Linux::Libstdcxx constants for SONAME/SOVERSION.
Library/Homebrew/test/macos_version_spec.rb Replace klass::... constant references with MacOSVersion::... constants.
Library/Homebrew/test/livecheck/strategy/launchpad_spec.rb Use fully qualified Homebrew::Livecheck::Strategy::Launchpad::DEFAULT_REGEX.
Library/Homebrew/test/livecheck/strategy/github_releases_spec.rb Bump to typed: true and use fully qualified ...GithubReleases::DEFAULT_REGEX.
Library/Homebrew/test/livecheck/strategy/extract_plist_spec.rb Use fully qualified ExtractPlist::Item constant instead of dynamic constant access.
Library/Homebrew/test/livecheck/strategy/crate_spec.rb Bump to typed: true and use fully qualified Crate::DEFAULT_REGEX.
Library/Homebrew/test/livecheck/strategy_spec.rb Bump to typed: true and use fully qualified Strategy::INVALID_BLOCK_RETURN_VALUE_MSG.
Library/Homebrew/test/keg_relocate/text_spec.rb Use Keg::Relocation instead of klass::Relocation.
Library/Homebrew/test/keg_relocate/grep_spec.rb Bump to typed: true.
Library/Homebrew/test/install_steps_spec.rb Use fully qualified Homebrew::InstallSteps::{DSL,Runner} rather than described_class::....
Library/Homebrew/test/github_runner_matrix_spec.rb Use GitHubRunnerMatrix::... constants directly to avoid dynamic constant access.
Library/Homebrew/test/formulary_spec.rb Replace dynamic loader class references with Formulary::From*Loader constants.
Library/Homebrew/test/env_config_spec.rb Bump to typed: true and use Homebrew::EnvConfig::ENVS.
Library/Homebrew/test/dependency_expansion_spec.rb Use Dependable::{PRUNE,SKIP,KEEP_BUT_PRUNE_RECURSIVE_DEPS} constants directly.
Library/Homebrew/test/completions_spec.rb Bump to typed: true and use Homebrew::Completions::SHELLS.
Library/Homebrew/test/cmd/which-formula_spec.rb Use fully qualified Homebrew::Cmd::WhichFormula::{ENDPOINT,DATABASE_FILE} constants.
Library/Homebrew/test/cmd/upgrade_spec.rb Use fully qualified nested constants under Homebrew::Cmd::UpgradeCmd.
Library/Homebrew/test/cmd/services/restart_subcommand_spec.rb Bump to typed: true and use fully qualified RestartSubcommand::TRIGGERS.
Library/Homebrew/test/cmd/services/list_subcommand_spec.rb Bump to typed: true and use fully qualified ListSubcommand::{TRIGGERS,JSON_FIELDS}.
Library/Homebrew/test/cmd/services/info_subcommand_spec.rb Bump to typed: true and use fully qualified InfoSubcommand::TRIGGERS.
Library/Homebrew/test/cmd/services/cleanup_subcommand_spec.rb Bump to typed: true and use fully qualified CleanupSubcommand::TRIGGERS.
Library/Homebrew/test/cmd/bundle_spec.rb Use fully qualified Homebrew::Cmd::Bundle::BUNDLE_EXTENSIONS.
Library/Homebrew/test/cask/list_spec.rb Use Cask::List::TAP_AND_NAME_COMPARISON directly instead of klass::....
Library/Homebrew/test/bundle/winget_spec.rb Use fully qualified Homebrew::Bundle::Winget::App constants.
Library/Homebrew/test/bundle/mac_app_store_spec.rb Use fully qualified Homebrew::Bundle::MacAppStore::App constants.
Library/Homebrew/test/bundle/brew_spec.rb Use fully qualified Homebrew::Bundle::Brew::Topo constant for mocking.
Library/Homebrew/test/attestation_spec.rb Use fully qualified Homebrew::Attestation constants and error classes.
Library/Homebrew/test/api/cask_struct_spec.rb Bump to typed: true and use fully qualified Homebrew::API::CaskStruct constants.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 31/31 changed files
  • Comments generated: 1

Comment thread Library/Homebrew/test/os/mac/sdk_spec.rb
Copy link
Copy Markdown
Member

@MikeMcQuaid MikeMcQuaid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I do wonder if we're doing this if it's worth just doing it everywhere instead of klass at all 🤷🏻

@MikeMcQuaid MikeMcQuaid added this pull request to the merge queue May 25, 2026
Merged via the queue into main with commit 3bdc5bf May 25, 2026
41 checks passed
@MikeMcQuaid MikeMcQuaid deleted the even-described-class-alternatives-are-still-bad-for-sorbet-sometimes branch May 25, 2026 20:22
@issyl0
Copy link
Copy Markdown
Member Author

issyl0 commented May 25, 2026

Thanks! I do wonder if we're doing this if it's worth just doing it everywhere instead of klass at all 🤷🏻

I'll gently and with a smile remind you of #22307 (review). We could do, though.

@MikeMcQuaid
Copy link
Copy Markdown
Member

@issyl0 To be fair, I did say "if we can use klass everywhere 😄. I think if we have to be inconsistent it's probably not worth it but don't feel strongly! 🤷🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants