feat(testing-system): add orthogonal decomposition evaluator MVP#107
feat(testing-system): add orthogonal decomposition evaluator MVP#107MeteorsLiu wants to merge 7 commits intogoplus:mainfrom
Conversation
MeteorsLiu
changed the title
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a robust testing framework that intelligently evaluates module build options. By tracing system calls during the build process, it constructs a detailed graph of actions and dependencies. This graph is then used to perform an orthogonal decomposition, identifying the minimal set of matrix combinations required for comprehensive testing, thereby significantly improving efficiency and reliability of module verification. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant new feature: a test command with an orthogonal decomposition evaluator to determine which matrix combinations require testing. This is a substantial piece of work involving build tracing, graph analysis, and complex heuristics. The overall approach is sophisticated and the implementation of the core evaluation logic is impressive. I've identified a few areas for improvement, mainly concerning code structure and maintainability, such as avoiding global state modifications between commands and reducing code duplication.
| savedVerbose, savedOutput := makeVerbose, makeOutput | ||
| makeVerbose, makeOutput = testVerbose, "" | ||
| defer func() { | ||
| makeVerbose, makeOutput = savedVerbose, savedOutput | ||
| }() |
There was a problem hiding this comment.
The testModule function modifies global variables (makeVerbose, makeOutput) defined in make.go. This creates a tight, implicit coupling between the test and make commands, which can make the code harder to reason about, debug, and maintain. It's better to refactor the shared functionality, like buildModuleWithRunTest, to accept all its dependencies as explicit parameters instead of relying on global state.
| matrix := formula.Matrix{ | ||
| Require: map[string][]string{ | ||
| "os": {runtime.GOOS}, | ||
| "arch": {runtime.GOARCH}, | ||
| }, | ||
| } | ||
| matrixStr = matrix.Combinations()[0] |
There was a problem hiding this comment.
This logic for creating a default matrix from the current runtime OS and architecture is duplicated in cmd/llar/internal/test.go. To improve maintainability and avoid code duplication, this logic should be extracted into a single, shared helper function. The defaultRuntimeMatrix() function in test.go could be moved to a shared location and reused here.
|
|
||
| profiles := make(map[string][]optionProfile, len(optionKeys)) | ||
| for _, key := range optionKeys { | ||
| values := slices.Clone(matrix.Options[key]) |
There was a problem hiding this comment.
The slice matrix.Options[key] is being cloned using slices.Clone, but the subsequent loop only reads from it. The clone is unnecessary and can be removed to simplify the code without changing the behavior.
| values := slices.Clone(matrix.Options[key]) | |
| values := matrix.Options[key] |
| if err != nil || info.IsDir() { | ||
| continue | ||
| } | ||
| sum, err := fileDigest(path) | ||
| if err != nil { | ||
| continue |
There was a problem hiding this comment.
Errors from os.Stat and fileDigest are being ignored. While this might be acceptable if some input files are expected to be transient, it could also mask underlying problems during the build trace analysis. Consider logging these errors for debugging purposes or adding a comment to clarify why they are being intentionally ignored.
![xgopilot[bot]](https://avatars.githubusercontent.com/in/1924301?s=60&v=4){kind=small}
| traceRoots, err := b.traceRoots(targets, mod, tmpSourceDir, installDir) | ||
| if err != nil { | ||
| return Result{}, err | ||
| } |
There was a problem hiding this comment.
os.Chdir() changes the CWD for the entire process. This is not safe if any other goroutine relies on CWD (and the TODO at line 362 plans parallel builds). Additionally, the original directory is never restored after Chdir — after Build() returns, the process CWD points to a deleted temp directory.
Consider passing the source directory to OnBuild rather than mutating process-global state, or at minimum restoring the CWD in a defer.
| func collectTraceInputDigests(records []trace.Record, scope trace.Scope) map[string]string { | ||
| buildRoot := filepath.Clean(scope.BuildRoot) | ||
| if buildRoot == "" || len(records) == 0 { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
os.Clearenv() removes all environment variables process-wide. Any concurrent goroutine reading env vars during the Clearenv() + Setenv loop will see a partially restored (or empty) environment. The TODO on line 354 acknowledges this — until a sandbox is introduced, this is a latent concurrency hazard. Consider documenting that Build() must not be called concurrently.
| func fileDigest(path string) (string, error) { | ||
| data, err := os.ReadFile(path) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| sum := sha256.Sum256(data) | ||
| return hex.EncodeToString(sum[:8]), nil |
There was a problem hiding this comment.
Two issues:
os.ReadFileloads the entire file into memory. For large build artifacts this is wasteful — streaming throughsha256.New()withio.Copywould bound memory.- The SHA-256 hash is silently truncated to 64 bits (
sum[:8]). This significantly increases collision probability (~50% at 2^32 files). If acceptable for this use case, the truncation should be documented.
| savedVerbose, savedOutput := makeVerbose, makeOutput | ||
| makeVerbose, makeOutput = testVerbose, "" | ||
| defer func() { | ||
| makeVerbose, makeOutput = savedVerbose, savedOutput |
There was a problem hiding this comment.
testModule reaches across to mutate make.go's package-level globals (makeVerbose, makeOutput) to reuse buildModuleWithRunTest. This tight coupling through shared mutable state is fragile — if the probe loop or build were ever parallelized, this would race. Consider passing these as explicit parameters to buildModuleWithRunTest instead.
| savedStdout, savedStderr := os.Stdout, os.Stderr | ||
| devNull, err := os.OpenFile(os.DevNull, os.O_WRONLY, 0) | ||
| if err != nil { | ||
| return evaluator.ProbeResult{}, fmt.Errorf("failed to open devnull for %s: %w", moduleArg, err) | ||
| } | ||
| defer func() { | ||
| devNull.Close() | ||
| os.Stdout = savedStdout | ||
| os.Stderr = savedStderr | ||
| }() | ||
| os.Stdout = devNull | ||
| os.Stderr = devNull |
There was a problem hiding this comment.
Global os.Stdout/os.Stderr redirection to /dev/null is not concurrency-safe. This same pattern also exists in make.go:147-158. Any goroutine writing to stdout/stderr (including panic handlers) during this window will have output silently lost. Consider passing io.Writer through the call chain rather than mutating process globals.
| } | ||
|
|
||
| func allowPtraceAttach() (func(), error) { | ||
| err := unix.Prctl(unix.PR_SET_PTRACER, uintptr(unix.PR_SET_PTRACER_ANY), 0, 0, 0) |
There was a problem hiding this comment.
PR_SET_PTRACER_ANY allows any same-UID process to ptrace the current process during the tracing window. A more targeted approach would use PR_SET_PTRACER with the specific strace child PID (start strace first, get its PID, then set the ptracer). If the chicken-and-egg problem prevents this, the security tradeoff should be documented with a comment here.
| func replaceScopeRootToken(token, root, placeholder string) string { | ||
| token = strings.ReplaceAll(token, root, placeholder) | ||
| if !strings.Contains(root, "$$TMP") { | ||
| return token | ||
| } | ||
| pattern := regexp.QuoteMeta(root) | ||
| pattern = strings.ReplaceAll(pattern, `\$\$TMP`, `[^/]+`) | ||
| re := regexp.MustCompile(pattern) | ||
| return re.ReplaceAllString(token, strings.ReplaceAll(placeholder, "$", "$$")) |
There was a problem hiding this comment.
regexp.MustCompile is called inside replaceScopeRootToken, which is invoked from normalizeScopeToken on every path of every action during graph construction and fingerprinting — potentially tens of thousands of times. Regex compilation is expensive (parsing + NFA construction). Since the root values come from a small fixed set (SourceRoot, BuildRoot, InstallRoot), these regexps should be compiled once per scope and cached/reused.
Similarly, normalizeScopeToken (line 1002-1018) allocates and sorts a 3-element struct slice on every call — this could also be precomputed per scope.
| if !slices.Contains(state.current.Changes, path) { | ||
| state.current.Changes = append(state.current.Changes, path) | ||
| } | ||
| } else { | ||
| if !slices.Contains(state.current.Inputs, path) { |
There was a problem hiding this comment.
slices.Contains is used to deduplicate Inputs and Changes on every open/rename/unlink syscall. This is O(k) per check where k grows with each file touch, yielding O(k^2) total per exec. A single compiler invocation can touch hundreds of files (include headers), making this quadratic behavior meaningful on large traces.
Consider using a map[string]struct{} side-set per Record for O(1) deduplication.
| flags := strings.ToUpper(strings.Join(call.args, " ")) | ||
| return strings.Contains(flags, "O_WRONLY") || | ||
| strings.Contains(flags, "O_RDWR") || | ||
| strings.Contains(flags, "O_TRUNC") || | ||
| strings.Contains(flags, "O_APPEND") || | ||
| (strings.Contains(flags, "O_CREAT") && strings.Contains(flags, "O_EXCL")) |
There was a problem hiding this comment.
isWriteOpen joins all call arguments (path, flags, mode) into one string and searches for flag names. This has two issues:
- Correctness: If a file path happens to contain
O_WRONLYas a substring, this produces a false positive. - Performance: This allocates a joined+uppercased string on every
open/openatsyscall — the most frequent events in a trace.
Consider examining only the flags argument directly (e.g., call.args[1] for open, call.args[2] for openat).
| reBuildTmpPIDNoise = regexp.MustCompile(`\.tmp\.[0-9]+($|/)`) | ||
| ) | ||
|
|
||
| func Watch(ctx context.Context, matrix formula.Matrix, probe ProbeFunc) ([]string, bool, error) { |
There was a problem hiding this comment.
Watch is the primary public entry point for the orthogonal decomposition evaluator — the core algorithm of this PR. It deserves a doc comment explaining: what it does (probes matrix combinations, diffs action graphs, finds collision components, returns minimal combo set), the meaning of the trusted return value, and any constraints on the probe function.
|
Review Summary Impressive feature — the orthogonal decomposition evaluator is architecturally well-conceived: tracing builds via strace, constructing action graphs, classifying tooling vs. business actions, then diffing to find colliding matrix options is a clever approach to minimizing test matrix execution. Key concerns:
See inline comments for details. |
1 participant
No description provided.