Import test new - #38
Conversation
This is a direct copy over with minor changes on import path
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive harness comparison evaluation suite under evals/harness-comparison/ to benchmark the generated Speakeasy CLI against a reference handwritten CLI, alongside a robust set of contract tests under tests/contract/ to validate CLI behaviors. The feedback is highly constructive, highlighting critical improvements for portability, robustness, and resource management. Specifically, the reviewer recommends quoting heredoc delimiters in setup.sh to avoid hardcoding absolute paths, adding error handling to the mock server's listen method to prevent hangs, ensuring proper cleanup of temporary directories to avoid leaks, and setting explicit UTF-8 encoding on process streams to prevent character corruption.
| cat > "$here/bin/generated/gemini-api" <<EOF | ||
| #!/usr/bin/env bash | ||
| for arg in "\$@"; do | ||
| if [[ "\$arg" == "@-" || "\$arg" == *=@- ]]; then | ||
| exec "$here/bin/generated/gemini-api-real" "\$@" | ||
| fi | ||
| done | ||
| exec "$here/bin/generated/gemini-api-real" "\$@" </dev/null | ||
| EOF |
There was a problem hiding this comment.
Using an unquoted heredoc delimiter (<<EOF) causes the $here variable to be expanded at setup time, hardcoding absolute paths into the generated bin/generated/gemini-api script. This makes the script non-portable if the repository is moved or run in a different environment (e.g., in a Docker container or a different CI runner path). Quoting the heredoc delimiter (<<'EOF') and dynamically resolving the path at runtime resolves this issue.
| cat > "$here/bin/generated/gemini-api" <<EOF | |
| #!/usr/bin/env bash | |
| for arg in "\$@"; do | |
| if [[ "\$arg" == "@-" || "\$arg" == *=@- ]]; then | |
| exec "$here/bin/generated/gemini-api-real" "\$@" | |
| fi | |
| done | |
| exec "$here/bin/generated/gemini-api-real" "\$@" </dev/null | |
| EOF | |
| cat > "$here/bin/generated/gemini-api" <<'EOF' | |
| #!/usr/bin/env bash | |
| here=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) | |
| for arg in "$@"; do | |
| if [[ "$arg" == "@-" || "$arg" == *=@- ]]; then | |
| exec "$here/gemini-api-real" "$@" | |
| fi | |
| done | |
| exec "$here/gemini-api-real" "$@" </dev/null | |
| EOF |
| cat > "$here/bin/reference/gemini-api-cli" <<EOF | ||
| #!/usr/bin/env bash | ||
| exec node "$reference_dir/dist/cli.js" "\$@" | ||
| EOF |
There was a problem hiding this comment.
Similarly, using an unquoted heredoc delimiter (<<EOF) here hardcodes the absolute path of $reference_dir at setup time. Quoting the heredoc delimiter (<<'EOF') and dynamically resolving the path relative to the script's location ensures portability.
| cat > "$here/bin/reference/gemini-api-cli" <<EOF | |
| #!/usr/bin/env bash | |
| exec node "$reference_dir/dist/cli.js" "\$@" | |
| EOF | |
| cat > "$here/bin/reference/gemini-api-cli" <<'EOF' | |
| #!/usr/bin/env bash | |
| here=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd) | |
| exec node "$here/../../reference/gemini-api-cli-poc/dist/cli.js" "$@" | |
| EOF |
| async start(): Promise<string> { | ||
| await new Promise<void>((resolve) => this.server.listen(0, "127.0.0.1", resolve)); | ||
| const addr = this.server.address() as AddressInfo; | ||
| this.baseUrl = `http://127.0.0.1:${addr.port}`; | ||
| return this.baseUrl; | ||
| } |
There was a problem hiding this comment.
If this.server.listen fails (e.g., due to port conflicts or other system errors), the returned promise will never resolve or reject, causing the test runner to hang indefinitely. Registering a temporary 'error' listener on the server during startup ensures that any listen errors are properly propagated and reject the promise.
async start(): Promise<string> {
await new Promise<void>((resolve, reject) => {
this.server.once("error", reject);
this.server.listen(0, "127.0.0.1", () => {
this.server.off("error", reject);
resolve();
});
});
const addr = this.server.address() as AddressInfo;
this.baseUrl = `http://127.0.0.1:${addr.port}`;
return this.baseUrl;
}| checkMatch(); | ||
| await server.stop(); |
There was a problem hiding this comment.
The temporary directories tmpConfig and cwd created via fs.mkdtempSync are never cleaned up, leading to a directory leak in the system's temporary folder on every evaluation run. Adding a cleanup block after stopping the server ensures these resources are properly reclaimed.
checkMatch();
await server.stop();
try {
fs.rmSync(tmpConfig, { recursive: true, force: true });
fs.rmSync(cwd, { recursive: true, force: true });
} catch {}| const child = spawn(python, [driver, driverCfgPath], { | ||
| stdio: ["ignore", "pipe", "pipe"], | ||
| detached: useProcessGroup, | ||
| }); |
There was a problem hiding this comment.
By default, child.stdout and child.stderr streams emit Buffer objects. Concatenating raw buffers directly to strings (e.g., buffered += d and stderr += d) can corrupt multi-byte UTF-8 characters if they happen to be split across chunk boundaries. Setting the encoding to 'utf8' on these streams prevents this issue.
| const child = spawn(python, [driver, driverCfgPath], { | |
| stdio: ["ignore", "pipe", "pipe"], | |
| detached: useProcessGroup, | |
| }); | |
| const child = spawn(python, [driver, driverCfgPath], { | |
| stdio: ["ignore", "pipe", "pipe"], | |
| detached: useProcessGroup, | |
| }); | |
| child.stderr.setEncoding("utf8"); | |
| child.stdout.setEncoding("utf8"); |
| func TestMain(m *testing.M) { | ||
| _, filename, _, ok := runtime.Caller(0) | ||
| if !ok { | ||
| fmt.Fprintln(os.Stderr, "resolve contract test path") | ||
| os.Exit(1) | ||
| } | ||
| root := filepath.Clean(filepath.Join(filepath.Dir(filename), "..", "..")) | ||
| tempDir, err := os.MkdirTemp("", "gemini-api-contract-") | ||
| if err != nil { | ||
| fmt.Fprintln(os.Stderr, err) | ||
| os.Exit(1) | ||
| } | ||
| defer os.RemoveAll(tempDir) | ||
|
|
||
| cliBinary = filepath.Join(tempDir, "gemini-api") | ||
| cmd := exec.Command("go", "build", "-o", cliBinary, "./cmd/gemini-api") | ||
| cmd.Dir = root | ||
| if output, err := cmd.CombinedOutput(); err != nil { | ||
| fmt.Fprintf(os.Stderr, "build gemini-api: %v\n%s", err, output) | ||
| os.Exit(1) | ||
| } | ||
|
|
||
| os.Exit(m.Run()) | ||
| } |
There was a problem hiding this comment.
In Go's TestMain, calling os.Exit bypasses any deferred functions (such as defer os.RemoveAll(tempDir)). This causes the temporary directory created via os.MkdirTemp to be leaked on every test run. Explicitly calling os.RemoveAll before exiting resolves this leak.
func TestMain(m *testing.M) {
_, filename, _, ok := runtime.Caller(0)
if !ok {
fmt.Fprintln(os.Stderr, "resolve contract test path")
os.Exit(1)
}
root := filepath.Clean(filepath.Join(filepath.Dir(filename), "..", ".."))
tempDir, err := os.MkdirTemp("", "gemini-api-contract-")
if err != nil {
fmt.Fprintln(os.Stderr, err)
os.Exit(1)
}
cliBinary = filepath.Join(tempDir, "gemini-api")
cmd := exec.Command("go", "build", "-o", cliBinary, "./cmd/gemini-api")
cmd.Dir = root
if output, err := cmd.CombinedOutput(); err != nil {
os.RemoveAll(tempDir)
fmt.Fprintf(os.Stderr, "build gemini-api: %v\n%s", err, output)
os.Exit(1)
}
code := m.Run()
os.RemoveAll(tempDir)
os.Exit(code)
}
This is the pr to import test and eval from the speakeasy cli repo