ci: add smoke test after building release binaries#161
Conversation
Verifies each platform binary executes, reports the correct version (ldflags injected), and handles the help subcommand before archiving. Closes #128 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
PR ReviewThis is a clean, minimal addition that directly addresses #128. The placement (between build and archive) is correct, and since the matrix runners match their target OS/arch, native execution works for all three platforms. Good work overall. One issue worth fixingRegex metacharacter risk in Version strings like - name: Smoke test
run: |
./tinkerdown version
./tinkerdown version | grep -Fq "${{ steps.version.outputs.version }}"
./tinkerdown helpThis is especially relevant if pre-release suffixes like Minor suggestion
- name: Smoke test
run: |
VERSION_OUT=$(./tinkerdown version)
echo "$VERSION_OUT"
echo "$VERSION_OUT" | grep -Fq "${{ steps.version.outputs.version }}"
./tinkerdown helpThis keeps the log output visible while only running the binary once for the version check. Looks good
|
There was a problem hiding this comment.
Pull request overview
Adds a lightweight smoke test to the CLI release workflow to validate the freshly built tinkerdown binary runs correctly (and reports the expected ldflags-injected version) before packaging and publishing release artifacts.
Changes:
- Insert a “Smoke test” step after the build step to run
./tinkerdown versionand./tinkerdown help - Add a version-string check against
${{ steps.version.outputs.version }}before creating the release archive
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - name: Smoke test | ||
| run: | | ||
| ./tinkerdown version | ||
| ./tinkerdown version | grep -q "${{ steps.version.outputs.version }}" |
There was a problem hiding this comment.
The version check uses grep without fixed-string mode, so dots in semver (e.g. 1.2.3) are treated as regex wildcards and could match an incorrect version output. Use fixed-string matching (e.g., grep -F) and ideally anchor to the full expected output to make this smoke test reliable.
| ./tinkerdown version | grep -q "${{ steps.version.outputs.version }}" | |
| ./tinkerdown version | grep -Fq "${{ steps.version.outputs.version }}" |
| ./tinkerdown version | ||
| ./tinkerdown version | grep -q "${{ steps.version.outputs.version }}" |
There was a problem hiding this comment.
./tinkerdown version is executed twice in this smoke test. Consider capturing the output once and reusing it for both printing and verification to keep the workflow step simpler and reduce redundant execution.
| ./tinkerdown version | |
| ./tinkerdown version | grep -q "${{ steps.version.outputs.version }}" | |
| VERSION_OUTPUT="$(./tinkerdown version)" | |
| echo "$VERSION_OUTPUT" | |
| echo "$VERSION_OUTPUT" | grep -q "${{ steps.version.outputs.version }}" |
Address review feedback: use grep -F so semver dots aren't treated as regex wildcards, and capture version output once instead of running the binary twice. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
ReviewThe smoke test step looks good. The current implementation already addresses both common pitfalls:
One small nit
OverallMinimal, well-placed, and correctly implemented. Good to merge once the test plan items are verified on the next tag push. |
Summary
./tinkerdown version), reports the correct ldflags-injected version, and handleshelpCloses #128
Test plan
workflow_dispatch🤖 Generated with Claude Code