Skip to content

docs: update README#65

Open
sayalibhavsar wants to merge 2 commits intomasterfrom
docs/update-readme
Open

docs: update README#65
sayalibhavsar wants to merge 2 commits intomasterfrom
docs/update-readme

Conversation

@sayalibhavsar
Copy link
Copy Markdown

Description

changes made to phoronix-wrapper documentation

Before/After Comparison

Changes include a template followed across all wrapper

Solves issue: #64
Relates to JIRA: RPOPC-940

@qodo-code-review
Copy link
Copy Markdown

Review Summary by Qodo

Comprehensive README documentation for Phoronix Test Suite wrapper

📝 Documentation

Grey Divider

Walkthroughs

Description
• Completely rewrote README with comprehensive documentation structure
• Added detailed description of wrapper functionality and supported sub-tests
• Documented all command-line options with clear formatting and defaults
• Added step-by-step workflow explanation of script execution
• Included dependency information for multiple OS variants
• Added usage examples and troubleshooting guidance
• Documented output files, result processing, and return codes
Diagram
flowchart LR
  A["Old README<br/>Basic description"] -->|"Complete rewrite"| B["New README<br/>Comprehensive docs"]
  B --> C["Description &<br/>Features"]
  B --> D["Command-line<br/>Options"]
  B --> E["Workflow<br/>Steps"]
  B --> F["Supported<br/>Sub-tests"]
  B --> G["Dependencies &<br/>Installation"]
  B --> H["Usage Examples &<br/>Troubleshooting"]
Loading

Grey Divider

File Changes

1. README.md 📝 Documentation +271/-37

Complete README rewrite with comprehensive documentation

• Replaced minimal 40-line README with comprehensive 274-line documentation
• Added structured sections with markdown headers for navigation
• Documented all 9 supported sub-tests (stress-ng, redis, cockroach, nginx, sqlite, phpbench,
 openssl, cassandra, apache-iotdb)
• Added detailed 9-step workflow explanation of script execution
• Included OS-specific dependency information for RHEL, Ubuntu, SLES, and Amazon Linux
• Added 6 practical usage examples with command variations
• Documented result processing, output files, return codes, and troubleshooting

README.md


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Copy Markdown

qodo-code-review Bot commented Apr 21, 2026

Code Review by Qodo

🐞 Bugs (3) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider


Action required

1. Validation only with PCP 🐞 Bug ≡ Correctness
Description
README claims JSON output and Pydantic schema validation are produced as part of normal result
processing, but run_phoronix.sh only runs csv_to_json/verify_results inside the --use_pcp
execution path. With PCP disabled (default), the script only produces CSV and skips JSON + schema
validation entirely.
Code

README.md[R11-13]

+- Result collection, processing, and verification.
+- CSV and JSON output formats with per-test Pydantic schema validation.
+- System configuration metadata capture.
Evidence
The README describes JSON generation and schema validation as standard output/processing, but the
script gates both conversion and validation behind if [[ $to_use_pcp -eq 1 ]]; the non-PCP path
only reduces to CSV and archives results without creating results_schema_*.json or running
verify_results.

README.md[7-16]
README.md[82-88]
README.md[226-230]
phoronix/run_phoronix.sh[412-435]
phoronix/run_phoronix.sh[481-487]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
README states JSON output and Pydantic schema validation happen as part of normal result processing, but the implementation only performs `csv_to_json` + `verify_results` when `--use_pcp` is enabled.

### Issue Context
Users running without PCP (default) will not get `results_schema_${sub_test}.json` nor any schema validation, contradicting README sections “Result Processing”, “Output Files”, and the feature list.

### Fix Focus Areas
- README.md[7-16]
- README.md[82-88]
- README.md[226-230]

### Proposed change
Update README wording to explicitly state that JSON generation + schema validation occur only when `--use_pcp` is enabled **or** (if the intent is to validate always) adjust the README to match a follow-up code change that moves validation outside the PCP conditional.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. PCP support overstated 🐞 Bug ≡ Correctness
Description
README states each sub-test has PCP/openmetrics definitions and test-specific PCP metric population,
but the script’s PCP handling is incomplete/buggy for some sub-tests. For example, cassandra’s PCP
branch is a placeholder and nginx PCP uses an undefined variable, so PCP metrics won’t match README
expectations.
Code

README.md[R121-124]

+## Supported Sub-Tests
+
+The wrapper supports the following Phoronix sub-tests, each with dedicated result parsing, schema validation, and PCP metric definitions:
+
Evidence
README claims per-sub-test PCP/openmetrics support; however, PCP setup assumes an
openmetrics_${test_name}_${sub_test}.txt file exists and the per-test PCP dispatch includes an
explicit cassandra placeholder and no apache-iotdb handling. Additionally, nginx PCP publishes
RPS:${RPS} even though the parsed variable is rps, so the emitted RPS value will be
empty/incorrect.

README.md[121-124]
README.md[255-258]
phoronix/run_phoronix.sh[276-285]
phoronix/run_phoronix.sh[437-451]
phoronix/run_phoronix.sh[353-364]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
README implies PCP/openmetrics definitions and PCP metric population are available for all supported sub-tests, but the implementation has gaps/bugs (e.g., cassandra placeholder, nginx RPS variable mismatch).

### Issue Context
This can mislead users enabling `--use_pcp` into expecting complete per-test PCP metrics.

### Fix Focus Areas
- README.md[121-124]
- README.md[255-258]

### Proposed change
Update README to list exactly which sub-tests have complete PCP metric support today (and call out any known gaps), or soften the language (e.g., “PCP support is available for select sub-tests”). Optionally, add a follow-up engineering task to implement missing PCP handlers and fix nginx’s `RPS` variable usage.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Advisory comments

3. Phoronix clone behavior mismatch 🐞 Bug ⚙ Maintainability
Description
README says Phoronix Test Suite is cloned fresh on each run, but run_phoronix.sh only clones if
./phoronix-test-suite does not already exist. This mismatch can lead users to assume a clean clone
when re-running in the same directory.
Code

README.md[R243-246]

+### Phoronix Test Suite Version
+- The wrapper uses Phoronix Test Suite version v10.8.1 (pinned for reproducibility).
+- The suite is cloned fresh on each run to ensure a clean state.
+
Evidence
The documentation claims a fresh clone every run, but the script explicitly guards cloning behind a
directory existence check.

README.md[243-246]
phoronix/run_phoronix.sh[237-239]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
README states the Phoronix Test Suite repo is cloned fresh on each run, but the script reuses an existing `./phoronix-test-suite` directory.

### Issue Context
This is a docs/behavior mismatch; users troubleshooting may chase the wrong assumptions about cached state.

### Fix Focus Areas
- README.md[243-246]

### Proposed change
Change the note to something like: “The suite is cloned if not already present in `./phoronix-test-suite` (delete the directory to force a fresh clone).”

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

Comment thread README.md
@sayalibhavsar sayalibhavsar requested a review from dvalinrh April 21, 2026 15:17
@sayalibhavsar sayalibhavsar self-assigned this Apr 21, 2026
Copy link
Copy Markdown
Contributor

@dvalinrh dvalinrh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also address the QODO comments. If it is a bug in our wrapper itself (indicates there could be), open an issue for them.

Comment thread README.md Outdated
Comment thread README.md
Comment thread README.md Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants