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

Add resolv-replace compatibility test #998

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions Gemfile
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ group :development, :test do
gem 'rubocop-performance'
gem 'rubocop-rake'
gem 'simplecov'

# For compatibility testing
gem 'resolv-replace', require: false
end

group :test do
Expand Down
87 changes: 87 additions & 0 deletions test/test_gem_compatibility.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
# frozen_string_literal: true

# JRuby does not support forking, and it doesn't seem worth the effort to make it work.
return unless Process.respond_to?(:fork)
Comment on lines +3 to +4
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Active Support has some workarounds for this we could use, but it doesn't seem worth it for a code path that was previously untested, and is unlikely to behave differently on JRuby.


require_relative 'helper'

describe 'gem compatibility' do
%w[
resolv-replace
].each do |gem_name|
it "passes smoke test with #{gem_name.inspect} gem required" do
memcached(:binary, rand(21_397..21_896)) do |_, port|
in_isolation(timeout: 10) do
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We want to be able to require the gem in isolation so as not to affect the host process and other tests.

before_client = Dalli::Client.new("127.0.0.1:#{port}")

assert_round_trip(before_client, "Failed to round-trip key before requiring #{gem_name.inspect}")

require gem_name

after_client = Dalli::Client.new("127.0.0.1:#{port}")

assert_round_trip(after_client, "Failed to round-trip key after requiring #{gem_name.inspect}")
end
end
end
end

private

def assert_round_trip(client, message)
expected = SecureRandom.hex(4)
key = "round-trip-#{expected}"
ttl = 10 # seconds

client.set(key, expected, ttl)

assert_equal(expected, client.get(key), message)
end

def in_isolation(timeout:) # rubocop:disable Metrics
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This method triggers several offences in the Metrics department, but I don't know that splitting it would be an improvement.

r, w = IO.pipe

pid = fork do
yield
exit!(0)
# We rescue Exception so we can catch everything, including MiniTest::Assertion.
rescue Exception => e # rubocop:disable Lint/RescueException
w.write(Marshal.dump(e))
ensure
w.close
exit!
end

begin
Timeout.timeout(timeout) do
_, status = Process.wait2(pid)
w.close
marshaled_exception = r.read
r.close

unless marshaled_exception.empty?
raise begin
Marshal.load(marshaled_exception) # rubocop:disable Security/MarshalLoad
rescue StandardError => e
raise <<~MESSAGE
Failed to unmarshal error from fork with exit status #{status.exitstatus}!
#{e.class}: #{e}
---MARSHALED_EXCEPTION---
#{marshaled_exception}
-------------------------
MESSAGE
end
end

unless status.success?
raise "Child process exited with non-zero status #{status.exitstatus} despite piping no exception"
end

pass
end
rescue Timeout::Error
Process.kill('KILL', pid)
raise "Child process killed after exceeding #{timeout}s timeout"
end
end
end
Loading