Emit Ninja manifest for Netsukefile test assertions#64
Merged
Conversation
Contributor
Reviewer's GuideThis PR updates the test workflow to emit Ninja manifests for all Netsukefile build steps and propagate the manifest path via Flow diagram for artifact assertion with Ninja manifest debuggingflowchart TD
A[Start assertion script] --> B{Does file exist?}
B -- Yes --> C[Success]
B -- No --> D{Is NINJA_MANIFEST set and file exists?}
D -- Yes --> E[Dump Ninja manifest to stderr]
D -- No --> F[Exit with error]
E --> F
F --> G[End]
C --> G
Flow diagram for assert-file-absent.sh scriptflowchart TD
A[Start assert-file-absent.sh] --> B{Does file exist?}
B -- No --> C[Success]
B -- Yes --> D{Is NINJA_MANIFEST set and file exists?}
D -- Yes --> E[Dump Ninja manifest to stderr]
D -- No --> F[Exit with error]
E --> F
F --> G[End]
C --> G
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Warning Rate limit exceeded@leynos has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 31 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
Summary by CodeRabbit
WalkthroughUpdate the GitHub workflow to emit Ninja manifest files during Changes
Sequence Diagram(s)sequenceDiagram
participant Workflow
participant Netsuke
participant AssertExists
participant AssertAbsent
Workflow->>Netsuke: Build/Action with --emit build.ninja/action.ninja
Netsuke-->>Workflow: Emit Ninja manifest file
Workflow->>AssertExists: Check for expected artefact (NINJA_MANIFEST set)
AssertExists-->>Workflow: Success or error (with manifest output on error)
Workflow->>AssertAbsent: Check for absent artefact (NINJA_MANIFEST set)
AssertAbsent-->>Workflow: Success or error (with manifest output on error)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey @leynos - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Add both behavioural and unit tests for the new assert-file-absent.sh script. (link)
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `scripts/assert-file-absent.sh:9` </location>
<code_context>
+
+if [[ $# -ne 1 ]]; then
+ echo "Usage: $(basename "$0") <file>" >&2
+ exit 64 # EX_USAGE
+fi
+
</code_context>
<issue_to_address>
Using exit code 64 is non-standard for shell scripts.
If portability is important, use a more widely recognized exit code such as 2 for usage errors.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
exit 64 # EX_USAGE
=======
exit 2 # usage error
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `scripts/assert-file-absent.sh:1` </location>
<code_context>
#!/usr/bin/env bash
# Ensures the Netsuke build produced the expected artefact.
-# Fails fast if the given file is missing.
</code_context>
<issue_to_address>
Add both behavioural and unit tests for the new assert-file-absent.sh script.
This new script introduces functionality to check for the absence of a file, but there are no accompanying behavioural or unit tests to verify its correct operation. Add tests to demonstrate and validate its behaviour.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -0,0 +1,23 @@ | |||
| #!/usr/bin/env bash | |||
Contributor
There was a problem hiding this comment.
issue (review_instructions): Add both behavioural and unit tests for the new assert-file-absent.sh script.
This new script introduces functionality to check for the absence of a file, but there are no accompanying behavioural or unit tests to verify its correct operation. Add tests to demonstrate and validate its behaviour.
Review instructions:
Path patterns: **/*
Instructions:
For any new feature or change to an existing feature, both behavioural and unit tests are required.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/netsukefile-test.yml(1 hunks)scripts/assert-file-absent.sh(1 hunks)scripts/assert-file-exists.sh(2 hunks)
Comment on lines
+56
to
+78
| env: | ||
| NINJA_MANIFEST: build.ninja | ||
| run: | | ||
| scripts/assert-file-exists.sh base.txt | ||
| scripts/assert-file-exists.sh dependent.txt | ||
| - name: Assert inline command artefact exists | ||
| env: | ||
| NINJA_MANIFEST: build.ninja | ||
| run: scripts/assert-file-exists.sh inline-command.txt | ||
| - name: Assert inline script artefact exists | ||
| env: | ||
| NINJA_MANIFEST: build.ninja | ||
| run: scripts/assert-file-exists.sh inline-script.txt | ||
| - name: Run action target | ||
| run: ./target/debug/netsuke --verbose build say-hello | ||
| run: ./target/debug/netsuke --verbose build --emit action.ninja say-hello | ||
| - name: Assert action artefact exists | ||
| env: | ||
| NINJA_MANIFEST: action.ninja | ||
| run: scripts/assert-file-exists.sh action-called.txt | ||
| - name: Assert unused action artefact absent | ||
| run: test ! -f unused.txt | ||
| env: | ||
| NINJA_MANIFEST: action.ninja | ||
| run: scripts/assert-file-absent.sh unused.txt |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Remove repetitive env: NINJA_MANIFEST blocks with YAML anchors or composite action
Four consecutive steps repeat identical env declarations. Use a YAML anchor/alias or wrap the assertions in a single composite step to shrink the workflow file and keep the manifest filename in one place.
Example with anchor:
env: &with-build-manifest
NINJA_MANIFEST: build.ninja
- name: Assert dependent artefacts exist
env: *with-build-manifest
run: |
scripts/assert-file-exists.sh base.txt
scripts/assert-file-exists.sh dependent.txt🤖 Prompt for AI Agents
In .github/workflows/netsukefile-test.yml between lines 56 and 78, multiple
steps redundantly declare the same env variable NINJA_MANIFEST with the value
build.ninja. Refactor by defining a YAML anchor for the env block with
NINJA_MANIFEST: build.ninja and then reference this anchor in each step that
requires it. This reduces repetition and centralizes the manifest filename
declaration.
Comment on lines
+16
to
+21
| if [[ -n "${NINJA_MANIFEST:-}" && -f "$NINJA_MANIFEST" ]]; then | ||
| echo "Ninja manifest '$NINJA_MANIFEST' for debugging:" >&2 | ||
| echo "-----BEGIN NINJA MANIFEST-----" >&2 | ||
| cat "$NINJA_MANIFEST" >&2 | ||
| echo "-----END NINJA MANIFEST-----" >&2 | ||
| fi |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Reuse the manifest-dump helper instead of duplicating logic
This block duplicates the one in assert-file-exists.sh. Source a common helper or declare a shared function to avoid future divergence and reduce surface area for bugs.
🤖 Prompt for AI Agents
In scripts/assert-file-absent.sh around lines 16 to 21, the code block that
prints the Ninja manifest duplicates logic found in assert-file-exists.sh.
Refactor by extracting this manifest-dump logic into a shared helper script or
function, then source or call that helper here to avoid duplication and ensure
consistency.
Comment on lines
+16
to
+21
| if [[ -n "${NINJA_MANIFEST:-}" && -f "$NINJA_MANIFEST" ]]; then | ||
| echo "Ninja manifest '$NINJA_MANIFEST' for debugging:" >&2 | ||
| echo "-----BEGIN NINJA MANIFEST-----" >&2 | ||
| cat "$NINJA_MANIFEST" >&2 | ||
| echo "-----END NINJA MANIFEST-----" >&2 | ||
| fi |
Contributor
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Extract duplicated Ninja-manifest dump into a helper to remove repetition
The manifest-dump block appears verbatim in assert-file-exists.sh and assert-file-absent.sh. Factor it into a small function or shared script to DRY the codebase and simplify maintenance.
- if [[ -n "${NINJA_MANIFEST:-}" && -f "$NINJA_MANIFEST" ]]; then
- echo "Ninja manifest '$NINJA_MANIFEST' for debugging:" >&2
- echo "-----BEGIN NINJA MANIFEST-----" >&2
- cat "$NINJA_MANIFEST" >&2
- echo "-----END NINJA MANIFEST-----" >&2
- fi
+ dump_ninja_manifest() {
+ echo "Ninja manifest '$NINJA_MANIFEST' for debugging:" >&2
+ echo "-----BEGIN NINJA MANIFEST-----" >&2
+ cat "$NINJA_MANIFEST" >&2
+ echo "-----END NINJA MANIFEST-----" >&2
+ }
+ [[ -n "${NINJA_MANIFEST:-}" && -f "$NINJA_MANIFEST" ]] && dump_ninja_manifest🤖 Prompt for AI Agents
In scripts/assert-file-exists.sh around lines 16 to 21, the code block that
dumps the Ninja manifest is duplicated in assert-file-absent.sh. Extract this
block into a helper function within a shared script or source it from a common
file. Replace the duplicated code in both scripts with calls to this helper
function to adhere to DRY principles and simplify maintenance.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
leynos
force-pushed
the
codex/add-emit-parameter-to-netsukefile-tests
branch
from
457abcc to
44d6576
Compare
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
--emitTesting
make fmtmake lintmake testhttps://chatgpt.com/codex/tasks/task_e_68929bba61548322b4639fd00b23c911
Summary by Sourcery
Enhance the Netsukefile test workflows to emit and persist Ninja manifests, improve assertion scripts to dump manifests on failure, and introduce a dedicated script for absent artefact checks.
Enhancements:
CI:
Tests: