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.

Sign up for GitHub

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

Jump to bottom

Livecheck: Extend strategy block support #10128

Merged
merged 2 commits into from Dec 28, 2020
Merged

Livecheck: Extend strategy block support #10128

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
361743e
Livecheck: Extend strategy block support
samford Dec 24, 2020
fef4512
Livecheck: Pass regex into strategy blocks
samford Dec 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions Library/Homebrew/livecheck/strategy/apache.rb
View file
Open in desktop
Expand Up @@ -38,7 +38,7 @@ def self.match?(url)
# @param url [String] the URL of the content to check
# @param regex [Regexp] a regex used for matching versions in content
# @return [Hash]
def self.find_versions(url, regex = nil)
def self.find_versions(url, regex = nil, &block)
%r{
path=
(?<path>.+?)/ # Path to directory of files or version directories
Expand All @@ -59,7 +59,7 @@ def self.find_versions(url, regex = nil)
# * `/href=["']?example-v?(\d+(?:\.\d+)+)-bin\.zip/i`
regex ||= /href=["']?#{Regexp.escape(prefix)}v?(\d+(?:\.\d+)+)#{Regexp.escape(suffix)}/i

PageMatch.find_versions(page_url, regex)
PageMatch.find_versions(page_url, regex, &block)
end
end
end
Expand Down
4 changes: 2 additions & 2 deletions Library/Homebrew/livecheck/strategy/bitbucket.rb
View file
Open in desktop
Expand Up @@ -52,7 +52,7 @@ def self.match?(url)
# @param url [String] the URL of the content to check
# @param regex [Regexp] a regex used for matching versions in content
# @return [Hash]
def self.find_versions(url, regex = nil)
def self.find_versions(url, regex = nil, &block)
match = url.match(URL_MATCH_REGEX)

# Use `\.t` instead of specific tarball extensions (e.g. .tar.gz)
Expand All @@ -71,7 +71,7 @@ def self.find_versions(url, regex = nil)
# * `/href=.*?example-v?(\d+(?:\.\d+)+)\.t/i`
regex ||= /href=.*?#{Regexp.escape(match[:prefix])}v?(\d+(?:\.\d+)+)#{Regexp.escape(suffix)}/i

PageMatch.find_versions(page_url, regex)
PageMatch.find_versions(page_url, regex, &block)
end
end
end
Expand Down
4 changes: 2 additions & 2 deletions Library/Homebrew/livecheck/strategy/cpan.rb
View file
Open in desktop
Expand Up @@ -37,7 +37,7 @@ def self.match?(url)
# @param url [String] the URL of the content to check
# @param regex [Regexp] a regex used for matching versions in content
# @return [Hash]
def self.find_versions(url, regex = nil)
def self.find_versions(url, regex = nil, &block)
%r{
(?<path>/authors/id(?:/[^/]+){3,}/) # Path before the filename
(?<prefix>[^/]+) # Filename text before the version
Expand All @@ -54,7 +54,7 @@ def self.find_versions(url, regex = nil)
# Example regex: `/href=.*?Brew[._-]v?(\d+(?:\.\d+)*)\.t/i`
regex ||= /href=.*?#{prefix}[._-]v?(\d+(?:\.\d+)*)#{Regexp.escape(suffix)}/i

PageMatch.find_versions(page_url, regex)
PageMatch.find_versions(page_url, regex, &block)
end
end
end
Expand Down
17 changes: 16 additions & 1 deletion Library/Homebrew/livecheck/strategy/git.rb
View file
Open in desktop
Expand Up @@ -74,7 +74,7 @@ def self.match?(url)
# @param url [String] the URL of the Git repository to check
# @param regex [Regexp] the regex to use for matching versions
# @return [Hash]
def self.find_versions(url, regex = nil)
def self.find_versions(url, regex = nil, &block)
match_data = { matches: {}, regex: regex, url: url }

tags_data = tag_info(url, regex)
Expand All @@ -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
samford marked this conversation as resolved.
Show resolved Hide resolved
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 😁

when String
match_data[:matches][value] = Version.new(value)
when Array
value.each do |tag|
match_data[:matches][tag] = Version.new(tag)
end
else
raise TypeError, "Return value of `strategy :git` block must be a string or array of strings."
end

return match_data
end

tags_data[:tags].each do |tag|
# Skip tag if it has a 'debian/' prefix and upstream does not do
# only 'debian/' prefixed tags
Expand Down
4 changes: 2 additions & 2 deletions Library/Homebrew/livecheck/strategy/github_latest.rb
View file
Open in desktop
Expand Up @@ -56,7 +56,7 @@ def self.match?(url)
# @param url [String] the URL of the content to check
# @param regex [Regexp] a regex used for matching versions in content
# @return [Hash]
def self.find_versions(url, regex = nil)
def self.find_versions(url, regex = nil, &block)
%r{github\.com/(?:downloads/)?(?<username>[^/]+)/(?<repository>[^/]+)}i =~ url.sub(/\.git$/i, "")

# Example URL: `https://github.com/example/example/releases/latest`
Expand All @@ -65,7 +65,7 @@ def self.find_versions(url, regex = nil)
# The default regex is the same for all URLs using this strategy
regex ||= %r{href=.*?/tag/v?(\d+(?:\.\d+)+)["' >]}i

PageMatch.find_versions(page_url, regex)
PageMatch.find_versions(page_url, regex, &block)
end
end
end
Expand Down
4 changes: 2 additions & 2 deletions Library/Homebrew/livecheck/strategy/gnome.rb
View file
Open in desktop
Expand Up @@ -36,7 +36,7 @@ def self.match?(url)
# @param url [String] the URL of the content to check
# @param regex [Regexp] a regex used for matching versions in content
# @return [Hash]
def self.find_versions(url, regex = nil)
def self.find_versions(url, regex = nil, &block)
%r{/sources/(?<package_name>.*?)/}i =~ url

page_url = "https://download.gnome.org/sources/#{package_name}/cache.json"
Expand All @@ -53,7 +53,7 @@ def self.find_versions(url, regex = nil)
# Example regex: `/example-(\d+\.([0-8]\d*?)?[02468](?:\.\d+)*?)\.t/i`
regex ||= /#{Regexp.escape(package_name)}-(\d+\.([0-8]\d*?)?[02468](?:\.\d+)*?)\.t/i

PageMatch.find_versions(page_url, regex)
PageMatch.find_versions(page_url, regex, &block)
end
end
end
Expand Down
4 changes: 2 additions & 2 deletions Library/Homebrew/livecheck/strategy/gnu.rb
View file
Open in desktop
Expand Up @@ -59,7 +59,7 @@ def self.match?(url)
# @param url [String] the URL of the content to check
# @param regex [Regexp] a regex used for matching versions in content
# @return [Hash]
def self.find_versions(url, regex = nil)
def self.find_versions(url, regex = nil, &block)
project_names = PROJECT_NAME_REGEXES.map do |project_name_regex|
m = url.match(project_name_regex)
m["project_name"] if m
Expand Down Expand Up @@ -89,7 +89,7 @@ def self.find_versions(url, regex = nil)
# Example regex: `%r{href=.*?example[._-]v?(\d+(?:\.\d+)*)(?:\.[a-z]+|/)}i`
regex ||= %r{href=.*?#{project_name}[._-]v?(\d+(?:\.\d+)*)(?:\.[a-z]+|/)}i

PageMatch.find_versions(page_url, regex)
PageMatch.find_versions(page_url, regex, &block)
end
end
end
Expand Down
4 changes: 2 additions & 2 deletions Library/Homebrew/livecheck/strategy/hackage.rb
View file
Open in desktop
Expand Up @@ -34,7 +34,7 @@ def self.match?(url)
# @param url [String] the URL of the content to check
# @param regex [Regexp] a regex used for matching versions in content
# @return [Hash]
def self.find_versions(url, regex = nil)
def self.find_versions(url, regex = nil, &block)
/^(?<package_name>.+?)-\d+/i =~ File.basename(url)

# A page containing a directory listing of the latest source tarball
Expand All @@ -43,7 +43,7 @@ def self.find_versions(url, regex = nil)
# Example regex: `%r{<h3>example-(.*?)/?</h3>}i`
regex ||= %r{<h3>#{Regexp.escape(package_name)}-(.*?)/?</h3>}i

PageMatch.find_versions(page_url, regex)
PageMatch.find_versions(page_url, regex, &block)
end
end
end
Expand Down
2 changes: 1 addition & 1 deletion Library/Homebrew/livecheck/strategy/header_match.rb
View file
Open in desktop
Expand Up @@ -45,7 +45,7 @@ def self.find_versions(url, regex, &block)
merged_headers = headers.reduce(&:merge)

if block
match = block.call(merged_headers)
match = block.call(merged_headers, regex)
else
match = nil

Expand Down
4 changes: 2 additions & 2 deletions Library/Homebrew/livecheck/strategy/launchpad.rb
View file
Open in desktop
Expand Up @@ -40,7 +40,7 @@ def self.match?(url)
# @param url [String] the URL of the content to check
# @param regex [Regexp] a regex used for matching versions in content
# @return [Hash]
def self.find_versions(url, regex = nil)
def self.find_versions(url, regex = nil, &block)
%r{launchpad\.net/(?<project_name>[^/]+)}i =~ url

# The main page for the project on Launchpad
Expand All @@ -49,7 +49,7 @@ def self.find_versions(url, regex = nil)
# The default regex is the same for all URLs using this strategy
regex ||= %r{class="[^"]*version[^"]*"[^>]*>\s*Latest version is (.+)\s*</}

PageMatch.find_versions(page_url, regex)
PageMatch.find_versions(page_url, regex, &block)
end
end
end
Expand Down
4 changes: 2 additions & 2 deletions Library/Homebrew/livecheck/strategy/npm.rb
View file
Open in desktop
Expand Up @@ -36,7 +36,7 @@ def self.match?(url)
# @param url [String] the URL of the content to check
# @param regex [Regexp] a regex used for matching versions in content
# @return [Hash]
def self.find_versions(url, regex = nil)
def self.find_versions(url, regex = nil, &block)
%r{registry\.npmjs\.org/(?<package_name>.+)/-/}i =~ url

page_url = "https://www.npmjs.com/package/#{package_name}?activeTab=versions"
Expand All @@ -46,7 +46,7 @@ def self.find_versions(url, regex = nil)
# * `%r{href=.*?/package/@example/example/v/(\d+(?:\.\d+)+)"}i`
regex ||= %r{href=.*?/package/#{Regexp.escape(package_name)}/v/(\d+(?:\.\d+)+)"}i

PageMatch.find_versions(page_url, regex)
PageMatch.find_versions(page_url, regex, &block)
end
end
end
Expand Down
2 changes: 1 addition & 1 deletion Library/Homebrew/livecheck/strategy/page_match.rb
View file
Open in desktop
Expand Up @@ -49,7 +49,7 @@ def self.match?(url)
# @return [Array]
def self.page_matches(content, regex, &block)
if block
case (value = block.call(content))
case (value = block.call(content, regex))
when String
return [value]
when Array
Expand Down
4 changes: 2 additions & 2 deletions Library/Homebrew/livecheck/strategy/pypi.rb
View file
Open in desktop
Expand Up @@ -36,7 +36,7 @@ def self.match?(url)
# @param url [String] the URL of the content to check
# @param regex [Regexp] a regex used for matching versions in content
# @return [Hash]
def self.find_versions(url, regex = nil)
def self.find_versions(url, regex = nil, &block)
/
(?<package_name>.+)- # The package name followed by a hyphen
.*? # The version string
Expand All @@ -55,7 +55,7 @@ def self.find_versions(url, regex = nil)
%r{href=.*?/packages.*?/#{Regexp.escape(package_name)}[._-]
v?(\d+(?:\.\d+)*(.post\d+)?)#{Regexp.escape(suffix)}}ix

PageMatch.find_versions(page_url, regex)
PageMatch.find_versions(page_url, regex, &block)
end
end
end
Expand Down
4 changes: 2 additions & 2 deletions Library/Homebrew/livecheck/strategy/sourceforge.rb
View file
Open in desktop
Expand Up @@ -50,7 +50,7 @@ def self.match?(url)
# @param url [String] the URL of the content to check
# @param regex [Regexp] a regex used for matching versions in content
# @return [Hash]
def self.find_versions(url, regex = nil)
def self.find_versions(url, regex = nil, &block)
if url.include?("/project")
%r{/projects?/(?<project_name>[^/]+)/}i =~ url
elsif url.include?(".net/p/")
Expand All @@ -66,7 +66,7 @@ def self.find_versions(url, regex = nil)
# create something that works for most URLs.
regex ||= %r{url=.*?/#{Regexp.escape(project_name)}/files/.*?[-_/](\d+(?:[-.]\d+)+)[-_/%.]}i

PageMatch.find_versions(page_url, regex)
PageMatch.find_versions(page_url, regex, &block)
end
end
end
Expand Down
4 changes: 2 additions & 2 deletions Library/Homebrew/livecheck/strategy/xorg.rb
View file
Open in desktop
Expand Up @@ -70,7 +70,7 @@ def self.match?(url)
# @param url [String] the URL of the content to check
# @param regex [Regexp] a regex used for matching versions in content
# @return [Hash]
def self.find_versions(url, regex)
def self.find_versions(url, regex = nil, &block)
file_name = File.basename(url)
/^(?<module_name>.+)-\d+/i =~ file_name

Expand All @@ -84,7 +84,7 @@ def self.find_versions(url, regex)

# Use the cached page content to avoid duplicate fetches
cached_content = @page_data[page_url]
match_data = PageMatch.find_versions(page_url, regex, cached_content)
match_data = PageMatch.find_versions(page_url, regex, cached_content, &block)

# Cache any new page content
@page_data[page_url] = match_data[:content] if match_data[:content].present?
Expand Down