Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Livecheck: Extend strategy block support #10128

Merged
merged 2 commits into from Dec 28, 2020
Merged

Livecheck: Extend strategy block support #10128

merged 2 commits into from Dec 28, 2020

Conversation

samford
Copy link
Member

@samford samford commented Dec 24, 2020

  • 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 style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run brew tests with your changes locally?
  • Have you successfully run brew man locally and committed any changes?

We recently added support for a strategy block in livecheck blocks as part of expanding support for casks. Originally, this support was only added to HeaderMatch, Sparkle and PageMatch.

This builds on that work by adding support for strategy blocks to the strategies that use PageMatch#find_versions internally (i.e., Apache, Bitbucket, Cpan, GithubLatest, Gnome, Gnu, Hackage, Npm, Pypi, and Sourceforge). I will be updating this PR to update Xorg in the same fashion after #10113 is merged.

Besides those strategies, this also adds support to the Git strategy. I've borrowed some related code from PageMatch#page_matches and adapted it to the context of Git#find_versions. There may be room for improvement in how this is handled but this is a straightforward first draft.


This also modifies the related instances of block.call (in HeaderMatch, PageMatch, and Git) to pass in a regex alongside the content (e.g., page content, tags, etc.). This allows us to access a regex generated within the strategy or defined in the livecheck block from within the strategy block.

Some early strategy blocks have been creating a regex within the strategy block but unfortunately this prevents us from getting at this information in the debug and verbose JSON output. When a strategy block only uses one regex for matching (and the matching is done in a manner that's similar to what the strategy normally does), it's arguably better to define the regex in the livecheck block instead.

@BrewTestBot
Copy link
Member

Review period will end on 2020-12-25 at 06:16:24 UTC.

@BrewTestBot BrewTestBot added the waiting for feedback Merging is blocked until sufficient time has passed for review label Dec 24, 2020
@BrewTestBot BrewTestBot removed the waiting for feedback Merging is blocked until sufficient time has passed for review label Dec 25, 2020
@BrewTestBot
Copy link
Member

Review period ended.

@samford samford marked this pull request as ready for review December 26, 2020 01:32
@@ -86,6 +86,21 @@ def self.find_versions(url, regex = nil)

tags_only_debian = tags_data[:tags].all? { |tag| tag.start_with?("debian/") }

if block
case (value = block.call(tags_data[:tags], regex))
Copy link
Member

Choose a reason for hiding this comment

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

@samford Might be nice to have some tests for these?

Copy link
Member Author

Choose a reason for hiding this comment

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

I would love to test more code in the livecheck strategies but we're currently limited by what we can exercise without making a network call (or stubbing).

The only way to test this at the moment would be to call Git#find_versions, which calls Git#tag_info, which makes a network call. We tried this before and it worked locally but inexplicably failed on CI. Adding this type of test would make CI unreliable and frustrating, so we simply avoid it.

However, I have two PRs planned that will improve this situation:

  • Add provided_content support to all the strategies (not just PageMatch). This continues to lay groundwork for caching response data in the future but also allows us to test the #find_versions methods by passing in static content (like we're doing in page_match_spec.rb).
  • Refactor the strategies to split the code that generates the page_url and default regex into a separate method (outside of #find_versions). This is needed to be able to implement smarter page caching (i.e., only storing page data if we'll need it again in a given run) but it also allows us to test this part of #find_versions.

Long story short, I can't add a test for this here but I'll be adding a test for it in a later PR. The aforementioned PRs are pretty simple, so I'm planning to have them up relatively soon (after I've wrapped up some others).

Copy link
Member

Choose a reason for hiding this comment

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

The only way to test this at the moment would be to call Git#find_versions, which calls Git#tag_info, which makes a network call. We tried this before and it worked locally but inexplicably failed on CI. Adding this type of test would make CI unreliable and frustrating, so we simply avoid it.

I think in this case it's worth stubbing, them?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think in this case it's worth stubbing

It makes sense to me but I've avoided it up to this point since you had qualms with stubbing when I initially suggested it. I agree that it's not ideal but I think it's at least better than the current situation, where we're not testing most livecheck code that involves network requests.

I'll work on this after the aforementioned PRs, as those will expand testing and we may be in a better position to decide how we should handle stubbing, where it makes sense, etc. However, if you have a particular approach in mind, let me know.

Copy link
Member

Choose a reason for hiding this comment

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

Reliable integration tests that hit the network > stubbed network requests > unreliable integration tests that hit the network > no tests 😁

@samford samford merged commit 8ab6922 into Homebrew:master Dec 28, 2020
@samford samford deleted the livecheck-extend-strategy-block-support branch December 28, 2020 17:25
@BrewTestBot BrewTestBot added the outdated PR was locked due to age label Jan 31, 2021
@Homebrew Homebrew locked as resolved and limited conversation to collaborators Jan 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants