Skip to content

Conversation

@samford
Copy link
Member

@samford samford commented Jan 27, 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?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew lgtm (style, typechecking and tests) with your changes locally?

This is a follow-up to #21335, where we modified livecheck's ExtractPlist strategy to also use url options from the livecheck block that are supported by Cask::URL's initializer. I manually tested that PR but adding tests was going to require some refactoring, so I figured I would handle it in a separate PR after merging.

This extracts the logic for creating a duplicate cask with a different artifact URL into a separate method, so we can properly test it. It's possible to exercise this in ExtractPlist.find_versions tests to some degree but isolating this code allows us to check the returned cask and ensure that this works like it should.

This also expands ExtractPlist tests, bringing coverage to 100% for both lines and branches. As part of getting to 100% coverage, I removed an old guard in find_versions that ensures cask is present, as that's covered by Sorbet and I can't exercise that condition.

I removed an existing find_versions test case which seemed to only check that Cask::CaskLoader.load works, as it wasn't testing ExtractPlist. Similarly, I removed the UnversionedCaskChecker expectations, as these tests should arguably be checking the output from ExtractPlist methods, not what's happening inside.


One other notable change is that this raises an error if an ExtractPlist livecheck block uses a URL option that isn't supported by Cask::URL. The existing code in ExtractPlist.find_versions quietly skips over livecheck block URL options that aren't supported by Cask::URL. That works but it doesn't make the issue apparent, so it may lead to a disconnect between what the livecheck block author expects and what actually happens.

Erroring will make the issue clear and ensure that only supported options are used. In the context of ExtractPlist, the URL options are solely intended to be passed to Cask::URL, so this should be fine for now.

Outside of that, I did some light refactoring of how we approached this, maintaining the existing behavior but making some tweaks.


Lastly, this updates the type signature for the @header value in Cask::URL and the accessor's return type. When Cask::URL.initialize is provided with a string argument for header, it creates an array containing the string. As a result, the type of @header can't be a string and will always be T.nilable(T::Array[String]. This is something that I noticed when writing tests for ExtractPlist, so I figured it wouldn't hurt to take care of it here but I can create a separate PR if needed.

When `Cask::URL.initialize` is provided with a string argument for
`header`, it creates an array containing the string. As a result, the
type of `@header` can't be a string and will always be
`T.nilable(T::Array[String]`. This updates the type signature for
`@header` and the accessor's return type.
The existing code in `ExtractPlist.find_versions` for collecting
`livecheck` block URL options that are supported by `Cask::URL` simply
skips over unsupported options. That works but it doesn't make the
issue apparent, so it may lead to a disconnect between what the
`livecheck` block author expects and what actually happens.

This modifies the code to error when `livecheck` block URL options are
used that `Cask::URL` doesn't support, as this makes the issue clear
and ensures that only supported options are used. In the context of
`ExtractPlist`, the URL options are solely intended to be passed to
`Cask::URL`, so this should be fine for now.
This maintains the same behavior of the logic for handling `livecheck`
block URL options but cleans it up a little. Namely:

* Use trailing `if` in `filter_map` instead of ternary with `nil`
branch.
* Use `options` instead of `cask.livecheck.options`, as these are the
same here.
* Use `select` with a `nil?` guard instead of `compact.each` and
pushing to an external variable, as this is a little tidier and uses
less memory allocation.
Copy link
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

Expands test coverage for the ExtractPlist livecheck strategy by refactoring URL-override cask creation into a dedicated method and adding fixtures to validate supported/unsupported livecheck URL options. Also tightens Sorbet typing for Cask::URL#header.

Changes:

  • Add ExtractPlist.cask_with_url helper and update find_versions to use it when an alternate URL is provided.
  • Expand ExtractPlist strategy specs and add new cask fixtures for livecheck URL option scenarios.
  • Update Cask::URL#header Sorbet types to reflect the runtime array normalization behavior.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
Library/Homebrew/livecheck/strategy/extract_plist.rb Extracts cask URL override logic into cask_with_url and reuses it from find_versions.
Library/Homebrew/test/livecheck/strategy/extract_plist_spec.rb Adds focused tests for cask_with_url and expands find_versions coverage.
Library/Homebrew/test/support/fixtures/cask/Casks/livecheck/livecheck-extract-plist.rb Adjusts fixture to use url :url and updated metadata text.
Library/Homebrew/test/support/fixtures/cask/Casks/livecheck/livecheck-extract-plist-with-url.rb New fixture to exercise livecheck URL string override behavior.
Library/Homebrew/test/support/fixtures/cask/Casks/livecheck/livecheck-extract-plist-with-supported-url-options.rb New fixture to validate supported URL options propagation to Cask::URL.
Library/Homebrew/test/support/fixtures/cask/Casks/livecheck/livecheck-extract-plist-with-unsupported-url-options.rb New fixture to validate erroring on unsupported URL options.
Library/Homebrew/cask/url.rb Updates Sorbet types for @header/header to match array normalization.

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

@samford samford force-pushed the livecheck/expand-extract_plist-tests branch 4 times, most recently from eff87ea to 527cb26 Compare January 28, 2026 00:16
Copy link
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!

@samford samford force-pushed the livecheck/expand-extract_plist-tests branch from 527cb26 to 7dc20cd Compare January 28, 2026 14:10
This extracts the logic for creating a duplicate cask with a different
artifact URL into a separate method, so we can properly test it. It's
possible to exercise this in `ExtractPlist.find_versions` tests to
some degree but isolating this code allows us to check the returned
cask and ensure that this works like it should.

This also expands `ExtractPlist` tests, bringing coverage to 100% for
both lines and branches. I removed an existing `find_versions` test
case which seemed to only check that `Cask::CaskLoader.load` works, as
it wasn't testing `ExtractPlist`. Similarly, I removed the
`UnversionedCaskChecker` expectations, as these tests should arguably
be checking the output from `ExtractPlist` methods, not what's
happening inside.
@samford samford force-pushed the livecheck/expand-extract_plist-tests branch from 7dc20cd to 0063bc4 Compare January 28, 2026 14:10
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.

4 participants