-
Notifications
You must be signed in to change notification settings - Fork 599
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
CI: Test for the health of all code base URLs #2127
Conversation
- Add a new unit test class for testing the health of all URLs defined in the code base. See the comments in `test/new_relic/healthy_urls_test.rb` for more details - Update `ci_cron.yml` to set a `CI_CRON` env var to true - Update `.ruboycop.yml` to permit `throw` and `catch` calls without parens - Add new `skip_unless_ci_cron` helper to permit tests to only run via `ci_cron.yml` - Add new `skip_unless_newest_ruby` helper to permit tests to only run on the newest Ruby - Add new `agent_root` helper to permit tests to quickly get to the agent root - Fix 5 URLs that the new test caught resolves #2126
Prevent the `env` tests from running the URL health check tests
always compare gem version objects
The new test will only run via CI cron, and only on the newest Ruby version available. Using a branch with a single bad URL, here is a demonstration: https://github.com/newrelic/newrelic-ruby-agent/actions/runs/5516187929 |
we only need to break out of the current loop, so throw/catch is overkill
SimpleCov Report
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like this test and the new helpers you've added. Approved as-is. Comments are more out of curiosity rather than a need for anything to change.
@@ -6,7 +6,7 @@ | |||
# each with slightly different APIs and semantics. | |||
# See: | |||
# http://www.deveiate.org/code/Ruby-MemCache/ (Gem: Ruby-MemCache) | |||
# http://seattlerb.rubyforge.org/memcache-client/ (Gem: memcache-client) | |||
# https://github.com/mperham/memcache-client (Gem: memcache-client) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't know he wrote memcache-client!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I'd be interested in knowing more about the history there. I think that repo is a fork of another fork of the original RubyForge code (which Mike also has commit access to). For whatever reason, it's what's linked to from the Rubygems.org page's "Homepage" link, so I'm using it here.
class HealthyUrlsTest < Minitest::Test | ||
ROOT = File.expand_path('../../..', __FILE__).freeze | ||
FILENAMES = %w[ | ||
baselines |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think I'm familiar with this file. Where is it and what does it do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's at test/performance/script/baselines
. I haven't checked, but possibly it's used to power the perf tests' setting of a baseline from which a subsequent set of results is compared to.
newrelic | ||
newrelic_cmd | ||
nrdebug |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, are these executables as well as files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, these ones are really just Ruby scripts so they're eligible for having URLs in the source code. For now I went with an allowlist approach of filenames and extensions for specifying what to scan for URLs. In the future we may want to reverse this and have a blocklist of things like image files or perhaps even take a quick peek at the file's encoding and make a judgment call from that.
js | ||
json | ||
md | ||
proto |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we make changes to the proto files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! Not only can we, but we have, and with a URL in fact! There is currently only 1 file at infinite_tracing/lib/proto/infinite_tracing.proto
. That file has a URL in the source code comments at the top.
Cool that you built this. |
end | ||
|
||
def gather_urls | ||
Dir.glob(File.join(ROOT, '**', '*')).each_with_object({}) do |file, urls| |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: Can default the urls
keys to be an empty array, ready to be << onto!
Example from docs: https://rubyapi.org/3.2/o/hash
h = Hash.new { |hash, key| "Default value for #{key}" }
Change we can do:
each_with_object(Hash.new { |h, k| h[k] = [] })
.
Which then would mean that urls[url] ||= []
could be omitted in line 155
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @olleolleolle! Implemented with #2132.
Using Hash.new([])
in conjunction with += []
from Ulysee's blog post looks pretty slick too, but the Ruby docs example approach is more readable and predictable in its behavior, so we'll go with it.
LETTERS = ('a'..'z').to_a
hash1 = LETTERS.each_with_object(Hash.new { |h, k| h[k] = [] }) { |l, h| h[l] << true }
hash2 = LETTERS.each_with_object(Hash.new([])) { |l, h| h[l] += [true] }
hash1 == hash2
Implement the [suggested improvement](#2127 (review)) for the URLs hash to have an empty files array for the hash value by default. Update an old library website to use RubyGems.org instead.
test/new_relic/healthy_urls_test.rb
for more detailsci_cron.yml
to set aCI_CRON
env var to true.ruboycop.yml
to permitthrow
andcatch
calls without parensskip_unless_ci_cron
helper to permit tests to only run viaci_cron.yml
skip_unless_newest_ruby
helper to permit tests to only run on the newest Rubyagent_root
helper to permit tests to quickly get to the agent rootresolves #2126