Livecheck: Extend strategy block support#10128
Livecheck: Extend strategy block support#10128samford merged 2 commits intoHomebrew:masterfrom samford:livecheck-extend-strategy-block-support
Conversation
|
Review period will end on 2020-12-25 at 06:16:24 UTC. |
|
Review period ended. |
| tags_only_debian = tags_data[:tags].all? { |tag| tag.start_with?("debian/") } | ||
|
|
||
| if block | ||
| case (value = block.call(tags_data[:tags], regex)) |
There was a problem hiding this comment.
@samford Might be nice to have some tests for these?
There was a problem hiding this comment.
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_contentsupport to all the strategies (not justPageMatch). This continues to lay groundwork for caching response data in the future but also allows us to test the#find_versionsmethods by passing in static content (like we're doing inpage_match_spec.rb). - Refactor the strategies to split the code that generates the
page_urland defaultregexinto 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).
There was a problem hiding this comment.
The only way to test this at the moment would be to call
Git#find_versions, which callsGit#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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Reliable integration tests that hit the network > stubbed network requests > unreliable integration tests that hit the network > no tests 😁
brew stylewith your changes locally?brew typecheckwith your changes locally?brew testswith your changes locally?brew manlocally and committed any changes?We recently added support for a
strategyblock inlivecheckblocks as part of expanding support for casks. Originally, this support was only added toHeaderMatch,SparkleandPageMatch.This builds on that work by adding support for
strategyblocks to the strategies that usePageMatch#find_versionsinternally (i.e.,Apache,Bitbucket,Cpan,GithubLatest,Gnome,Gnu,Hackage,Npm,Pypi, andSourceforge). I will be updating this PR to updateXorgin the same fashion after #10113 is merged.Besides those strategies, this also adds support to the
Gitstrategy. I've borrowed some related code fromPageMatch#page_matchesand adapted it to the context ofGit#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(inHeaderMatch,PageMatch, andGit) 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 thelivecheckblock from within thestrategyblock.Some early
strategyblocks have been creating a regex within thestrategyblock but unfortunately this prevents us from getting at this information in the debug and verbose JSON output. When astrategyblock 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 thelivecheckblock instead.