Tidy tests & internal#53
Merged
Merged
Conversation
snaggle & snaggle_test have same logger
Tests run:
- first snaggle, then snaggle_test
- Then in lexical order of Testfunc
- Subtests run in order of test table
internal_test has different logger
This reverts commit 9ac73c5.
Reviewer's GuideThis PR centralizes test fixtures and utilities by extracting common ELF definitions, introducing helper functions for expected outputs and file comparisons, refactoring tests to use these new helpers, and encapsulating path and logging logic in new internal packages. Class diagram for updated binaryDetails and elf.Elf usageclassDiagram
class binaryDetails {
string Description
elf.Elf Elf
bool Dynamic
bool Exe
bool Lib
bool HasInterpreter
}
class Elf {
string Name
string Path
EI_CLASS Class
Type Type
string Interpreter
string[] Dependencies
}
binaryDetails --> Elf : Elf
Class diagram for new internal comparison helpersclassDiagram
class internal {
+bool SameFile(path1 string, path2 string)
+void AssertSameFile(t *testing.T, path1 string, path2 string, mustBeLink bool)
+bool sameFile(path1 string, path2 string)
+bool sameInode(path1 string, path2 string)
+bool sameFilemode(path1 string, path2 string)
+bool sameHash(path1 string, path2 string)
+[]byte hashFile(path string)
}
Class diagram for new internal path and logging helpersclassDiagram
class internal {
+string TestdataPath(path string)
+string WorkspaceTempDir(t testing.TB)
+*os.File PermissionDenied(t *testing.T, filename string)
+string LinkMessage(from string, to string)
+[]string StripLines(multiline string)
+string findLib(filename string)
+regexp.Regexp Ld_linux_64_RE
+const string P_ld_linux
+string P_libc
+string P_libm
+string P_libpcre2_8
+string P_libpthread
+string P_libselinux
+string P_hello_pie
+string P_hello_static
+string P_which
+string P_id
+string P_ldd
+string P_ctypes_so
}
Class diagram for refactored CommonBinaries and commonElfsclassDiagram
class CommonBinaries {
+map<string, binaryDetails> CommonBinaries(t testing.TB)
}
class commonElfs {
+map<string, elf.Elf> commonElfs
}
CommonBinaries --> binaryDetails
binaryDetails --> Elf
commonElfs --> Elf
Class diagram for new ExpectedOutput helperclassDiagram
class ExpectedOutput {
+([]string, map[string]string) ExpectedOutput(tc binaryDetails, tmp string)
}
ExpectedOutput --> binaryDetails
binaryDetails --> Elf
Class diagram for new libpathcmp functionclassDiagram
class Elf {
+[]string Diff(o Elf)
}
class libpathcmp {
+int libpathcmp(path1 string, path2 string)
}
Elf --> libpathcmp : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #53 +/- ##
==========================================
- Coverage 74.13% 73.86% -0.28%
==========================================
Files 5 8 +3
Lines 406 417 +11
==========================================
+ Hits 301 308 +7
- Misses 71 77 +6
+ Partials 34 32 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `internal/comparisons.go:37` </location>
<code_context>
+ assert.Truef(t, same, "%s & %s are different files", path1, path2)
+}
+
+func sameFile(path1 string, path2 string) (bool, error) {
+ same, err := sameInode(path1, path2)
+ switch {
</code_context>
<issue_to_address>
**suggestion (performance):** Consider short-circuiting on file size before hashing.
Checking file sizes first can reduce unnecessary hashing and improve performance, especially with large files.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This was
linked to
issues
Oct 26, 2025
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Centralize ELF test data and file utilities into internal packages, introduce a portable libpathcmp comparator, and refactor tests across the codebase to leverage shared helpers for DRY and consistency
New Features:
Enhancements:
Tests: