Adding more tests#8
Open
LowLevelLore wants to merge 2 commits into
Open
Conversation
Reviewer's GuideThis PR adds a Python helper script to automatically generate expected runtime test outputs and introduces a broad set of new runtime tests (with corresponding expected stdout/exitcode files) for conditionals, functions, loops, operations, strings, types, and variables, while wiring them into the existing expected-results structure. Sequence diagram for generate_expected_outputs runtime flowsequenceDiagram
actor Developer
participant Script as generate_expected_outputs_py
participant Zpiler as ZPILER
participant As as as
participant Gcc as gcc
participant Exe as test_executable
participant FS as Filesystem
Developer->>Script: main()
Script->>FS: RUNTIME_DIR.rglob
loop for each test_file
Script->>Script: generate_outputs_for_test(test_file)
Script->>Zpiler: run_command(ZPILER ...)
alt compile ok
Script->>As: run_command(as asm_file -o obj_file)
alt assemble ok
Script->>Gcc: run_command(gcc obj_file -o exe_file)
alt link ok
Script->>Exe: run_command(exe_file)
Script->>FS: write_bytes(.stdout/.stderr/.exitcode)
else link failed
Script-->>Script: return False
end
else assemble failed
Script-->>Script: return False
end
else compile failed
Script-->>Script: return False
end
end
Script->>Developer: print summary of success_count/fail_count
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 security issue, 2 other issues, and left some high level feedback:
Security issues:
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
General comments:
- The hard-coded absolute paths for
ZPILER,TESTS_DIR, and related dirs ingenerate_expected_outputs.pymake the script environment-specific; consider deriving these from the repository root or accepting them via CLI args so it can run on different machines/CI environments. - In
generate_expected_outputs.py,run_commandtakes acheckflag but still exits the whole script on failure whencheck=True; for more flexible use (e.g., partial regeneration), you might want to have it raise an exception or return error details instead of callingsys.exit(1)directly. - The script currently emits only a generic 'FAILED to compile/assemble/link' message; including the underlying
stderrfrom those failing subprocesses in those branches would make it much easier to debug failing test generation.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The hard-coded absolute paths for `ZPILER`, `TESTS_DIR`, and related dirs in `generate_expected_outputs.py` make the script environment-specific; consider deriving these from the repository root or accepting them via CLI args so it can run on different machines/CI environments.
- In `generate_expected_outputs.py`, `run_command` takes a `check` flag but still exits the whole script on failure when `check=True`; for more flexible use (e.g., partial regeneration), you might want to have it raise an exception or return error details instead of calling `sys.exit(1)` directly.
- The script currently emits only a generic 'FAILED to compile/assemble/link' message; including the underlying `stderr` from those failing subprocesses in those branches would make it much easier to debug failing test generation.
## Individual Comments
### Comment 1
<location path="generate_expected_outputs.py" line_range="5-11" />
<code_context>
+import tempfile
+import shutil
+
+ZPILER = Path("/home/xzist/zust/build/zpiler")
+TESTS_DIR = Path("/home/xzist/zust/tests")
+RUNTIME_DIR = TESTS_DIR / "runtime"
+EXPECTED_DIR = TESTS_DIR / "expected" / "runtime"
+
+def run_command(cmd, check=True):
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Avoid hardcoded absolute paths to make the script portable within the repo
Using `/home/xzist/...` makes this script unusable outside your machine and likely to fail in CI. Instead, derive these paths relative to this file (e.g. via `Path(__file__).resolve()` to the repo root, then into `build/zpiler` and `tests`) so it works for all environments.
```suggestion
import tempfile
import shutil
# Derive repository-root-relative paths so the script is portable
SCRIPT_PATH = Path(__file__).resolve()
REPO_ROOT = SCRIPT_PATH
while not ((REPO_ROOT / "build").is_dir() and (REPO_ROOT / "tests").is_dir()):
if REPO_ROOT.parent == REPO_ROOT:
raise RuntimeError(
"Could not locate repository root containing 'build' and 'tests' directories"
)
REPO_ROOT = REPO_ROOT.parent
ZPILER = REPO_ROOT / "build" / "zpiler"
TESTS_DIR = REPO_ROOT / "tests"
RUNTIME_DIR = TESTS_DIR / "runtime"
EXPECTED_DIR = TESTS_DIR / "expected" / "runtime"
```
</issue_to_address>
### Comment 2
<location path="generate_expected_outputs.py" line_range="39-48" />
<code_context>
+
+ # Compile
+ print(f"Compiling {rel_path}...", end=" ")
+ result = run_command([str(ZPILER), "--format", "x86_64-linux", "-o", str(asm_file), str(test_file)], check=False)
+ if result.returncode != 0:
+ print(f"FAILED to compile")
+ return False
+
+ # Assemble
+ result = run_command(["as", str(asm_file), "-o", str(obj_file)], check=False)
+ if result.returncode != 0:
+ print(f"FAILED to assemble")
+ return False
+
+ # Link
+ result = run_command(["gcc", str(obj_file), "-o", str(exe_file)], check=False)
+ if result.returncode != 0:
+ print(f"FAILED to link")
+ return False
+
</code_context>
<issue_to_address>
**suggestion:** Surface more diagnostic info when compile/assemble/link steps fail
Because `check=False` is used, failures here only show a generic `FAILED to ...` message and hide the underlying error output, which makes debugging test failures difficult. Please either use `check=True` so `run_command` prints stdout/stderr on failure, or explicitly log `result.stdout` and `result.stderr` in these non‑zero return cases.
Suggested implementation:
```python
# Compile
print(f"Compiling {rel_path}...", end=" ")
result = run_command(
[str(ZPILER), "--format", "x86_64-linux", "-o", str(asm_file), str(test_file)],
check=True,
)
if result.returncode != 0:
print("FAILED to compile")
return False
# Assemble
result = run_command(["as", str(asm_file), "-o", str(obj_file)], check=True)
if result.returncode != 0:
print("FAILED to assemble")
return False
# Link
result = run_command(["gcc", str(obj_file), "-o", str(exe_file)], check=True)
if result.returncode != 0:
print("FAILED to link")
return False
```
```python
def run_command(cmd, check=True):
result = subprocess.run(cmd, capture_output=True, text=False)
if check and result.returncode != 0:
print(f"Command failed: {' '.join(cmd)}")
if result.stdout:
print(f"stdout:\n{result.stdout.decode(errors='replace')}")
if result.stderr:
print(f"stderr:\n{result.stderr.decode(errors='replace')}")
```
If `subprocess` is not already imported at the top of `generate_expected_outputs.py`, you should add:
```python
import subprocess
```
near the other imports.
</issue_to_address>
### Comment 3
<location path="generate_expected_outputs.py" line_range="14" />
<code_context>
result = subprocess.run(cmd, capture_output=True, text=False)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+5
to
+11
| import tempfile | ||
| import shutil | ||
|
|
||
| ZPILER = Path("/home/xzist/zust/build/zpiler") | ||
| TESTS_DIR = Path("/home/xzist/zust/tests") | ||
| RUNTIME_DIR = TESTS_DIR / "runtime" | ||
| EXPECTED_DIR = TESTS_DIR / "expected" / "runtime" |
There was a problem hiding this comment.
suggestion (bug_risk): Avoid hardcoded absolute paths to make the script portable within the repo
Using /home/xzist/... makes this script unusable outside your machine and likely to fail in CI. Instead, derive these paths relative to this file (e.g. via Path(__file__).resolve() to the repo root, then into build/zpiler and tests) so it works for all environments.
Suggested change
| import tempfile | |
| import shutil | |
| ZPILER = Path("/home/xzist/zust/build/zpiler") | |
| TESTS_DIR = Path("/home/xzist/zust/tests") | |
| RUNTIME_DIR = TESTS_DIR / "runtime" | |
| EXPECTED_DIR = TESTS_DIR / "expected" / "runtime" | |
| import tempfile | |
| import shutil | |
| # Derive repository-root-relative paths so the script is portable | |
| SCRIPT_PATH = Path(__file__).resolve() | |
| REPO_ROOT = SCRIPT_PATH | |
| while not ((REPO_ROOT / "build").is_dir() and (REPO_ROOT / "tests").is_dir()): | |
| if REPO_ROOT.parent == REPO_ROOT: | |
| raise RuntimeError( | |
| "Could not locate repository root containing 'build' and 'tests' directories" | |
| ) | |
| REPO_ROOT = REPO_ROOT.parent | |
| ZPILER = REPO_ROOT / "build" / "zpiler" | |
| TESTS_DIR = REPO_ROOT / "tests" | |
| RUNTIME_DIR = TESTS_DIR / "runtime" | |
| EXPECTED_DIR = TESTS_DIR / "expected" / "runtime" |
Comment on lines
+39
to
+48
| result = run_command([str(ZPILER), "--format", "x86_64-linux", "-o", str(asm_file), str(test_file)], check=False) | ||
| if result.returncode != 0: | ||
| print(f"FAILED to compile") | ||
| return False | ||
|
|
||
| # Assemble | ||
| result = run_command(["as", str(asm_file), "-o", str(obj_file)], check=False) | ||
| if result.returncode != 0: | ||
| print(f"FAILED to assemble") | ||
| return False |
There was a problem hiding this comment.
suggestion: Surface more diagnostic info when compile/assemble/link steps fail
Because check=False is used, failures here only show a generic FAILED to ... message and hide the underlying error output, which makes debugging test failures difficult. Please either use check=True so run_command prints stdout/stderr on failure, or explicitly log result.stdout and result.stderr in these non‑zero return cases.
Suggested implementation:
# Compile
print(f"Compiling {rel_path}...", end=" ")
result = run_command(
[str(ZPILER), "--format", "x86_64-linux", "-o", str(asm_file), str(test_file)],
check=True,
)
if result.returncode != 0:
print("FAILED to compile")
return False
# Assemble
result = run_command(["as", str(asm_file), "-o", str(obj_file)], check=True)
if result.returncode != 0:
print("FAILED to assemble")
return False
# Link
result = run_command(["gcc", str(obj_file), "-o", str(exe_file)], check=True)
if result.returncode != 0:
print("FAILED to link")
return Falsedef run_command(cmd, check=True):
result = subprocess.run(cmd, capture_output=True, text=False)
if check and result.returncode != 0:
print(f"Command failed: {' '.join(cmd)}")
if result.stdout:
print(f"stdout:\n{result.stdout.decode(errors='replace')}")
if result.stderr:
print(f"stderr:\n{result.stderr.decode(errors='replace')}")If subprocess is not already imported at the top of generate_expected_outputs.py, you should add:
import subprocessnear the other imports.
| EXPECTED_DIR = TESTS_DIR / "expected" / "runtime" | ||
|
|
||
| def run_command(cmd, check=True): | ||
| result = subprocess.run(cmd, capture_output=True, text=False) |
There was a problem hiding this comment.
security (python.lang.security.audit.dangerous-subprocess-use-audit): Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
Source: opengrep
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
Add tooling and fixtures to expand runtime test coverage across conditionals, functions, loops, operations, strings, types, and variables.
Enhancements:
Tests: