Conversation
raggi
force-pushed
the
raggi/fix-ruby
branch
2 times, most recently
from
f85ad5f to
44bcd8d
Compare
There was a problem hiding this comment.
Pull request overview
Updates the Ruby test harness to use a testcontrol instance (from tailscale.com/cmd/testcontrol) and adjusts build/test plumbing so the Ruby bindings can run integration-style network tests reliably.
Changes:
- Start a global
testcontrolprocess for Ruby tests and wire tests to use its control URL. - Refactor Ruby tests to use ephemeral nodes with per-test state directories.
- Misc cleanup: Makefile target hygiene, Ruby style formatting, gem updates, and ignore rules.
Reviewed changes
Copilot reviewed 5 out of 7 changed files in this pull request and generated 14 comments.
Show a summary per file
| File | Description |
|---|---|
tstestcontrol/Makefile |
Makes the archive build target explicit and adds .PHONY. |
ruby/test/test_helper.rb |
Installs/spawns testcontrol and switches Minitest bootstrapping. |
ruby/test/tailscale/test_tailscale.rb |
Uses $testcontrol_url, adds tmp state dir, and brings nodes up via up. |
ruby/lib/tailscale.rb |
Large formatting refactor of the Ruby FFI wrapper. |
ruby/ext/libtailscale/extconf.rb |
Updates Makefile generation style/quoting. |
ruby/Gemfile.lock |
Bumps Ruby dev/test dependencies and adds a platform entry. |
.gitignore |
Ignores libtailscale.bundle. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
raggi
force-pushed
the
raggi/fix-ruby
branch
9 times, most recently
from
3734872 to
77a51d2
Compare
The test helper ends up starting a single global instance of testcontrol which is a bit of a hack, but signal handling conflicts between the runtimes are otherwise problematic. Fixes tailscale/tailscale#18635
| end No newline at end of file | ||
| f.puts("install: libtailscale.#{RbConfig::CONFIG["DLEXT"]}") | ||
| f.puts("\tmkdir -p \"#{RbConfig::CONFIG["sitelibdir"]}\"") | ||
| f.puts("\tcp libtailscale.#{RbConfig::CONFIG["DLEXT"]} \"#{RbConfig::CONFIG["sitelibdir"]}/\"") |
There was a problem hiding this comment.
I'm not familiar with the style guide for this project, but I assume this is meant to keep this file in line with the guide/linter; escapes look correct to me, and probably better to standardize on "
| require "rbconfig" | ||
| require "net/http" | ||
| require "json" | ||
| require "base64" |
There was a problem hiding this comment.
Nit, you can alpha sort these if you like, but not a hill I'd die on:
require "base64"
require "ffi"
require "json"
require "net/http"
require "rbconfig"
require "tailscale/version"| path = gobin + "/testcontrol" | ||
| if !File.executable? path | ||
| raise "#{path} is not an executable" | ||
| end |
There was a problem hiding this comment.
Some small style changes, feel free to ignore:
gobin = `go env GOBIN`.strip
gobin = "#{`go env GOPATH`.strip}/bin" if gobin.empty?
path = "#{gobin}/testcontrol"
raise "#{path} is not an executable" unless File.executable? path| rescue Errno::ESRCH | ||
| raise "testcontrol exited prematurely" | ||
| end | ||
| if attempts == 10000 |
There was a problem hiding this comment.
Worth making the sleep duration and attempt limit top-level constants?
|
Ah, @nickmarrone beat me to the merge! No worries, all optional notes and looks good. Thank you! 🙏 |
3 participants
The test helper ends up starting a single global instance of testcontrol
which is a bit of a hack, but signal handling conflicts between the
runtimes are otherwise problematic.