Skip to content

Emit Ninja manifest for Netsukefile test assertions #64

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
leynos merged 2 commits into from
Aug 6, 2025
+44 −4
Merged

Emit Ninja manifest for Netsukefile test assertions #64

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c72878c
Extract unused artefact assertion into script
leynos Aug 6, 2025
44d6576
Use widely recognized exit code 2 for usage errors
leynos Aug 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions .github/workflows/netsukefile-test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,18 +51,28 @@ jobs:
command: "touch unused.txt"
MANIFEST
- name: Build dependent and inline targets
run: ./target/debug/netsuke --verbose build dependent.txt inline-command.txt inline-script.txt
run: ./target/debug/netsuke --verbose build --emit build.ninja dependent.txt inline-command.txt inline-script.txt
- name: Assert dependent artefacts exist
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
Comment on lines +56 to +78
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Aug 6, 2025

Choose a reason for hiding this comment

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

🧹 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.

23 changes: 23 additions & 0 deletions scripts/assert-file-absent.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
#!/usr/bin/env bash
Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai Bot Aug 6, 2025

Choose a reason for hiding this comment

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

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.

# Ensures the Netsuke build did not produce an unexpected artefact.
# If the artefact is present and `NINJA_MANIFEST` is set, the referenced
# Ninja manifest is dumped to stderr for debugging.
set -euo pipefail

if [[ $# -ne 1 ]]; then
echo "Usage: $(basename "$0") <file>" >&2
exit 2 # usage error
fi

file="$1"

if [[ -f "$file" ]]; then
echo "Unexpected build artefact '$file' present." >&2
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
Comment on lines +16 to +21
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Aug 6, 2025

Choose a reason for hiding this comment

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

🧹 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.

exit 1
fi
9 changes: 8 additions & 1 deletion scripts/assert-file-exists.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
#!/usr/bin/env bash
# Ensures the Netsuke build produced the expected artefact.
# Fails fast if the given file is missing.
# If the artefact is absent and `NINJA_MANIFEST` is set, the referenced
# Ninja manifest is dumped to stderr for debugging.
set -euo pipefail

if [[ $# -ne 1 ]]; then
Expand All @@ -12,5 +13,11 @@ file="$1"

if [[ ! -f "$file" ]]; then
echo "Expected build artefact '$file' to exist." >&2
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
Comment on lines +16 to +21
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Aug 6, 2025

Choose a reason for hiding this comment

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

🧹 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.

exit 1
fi
Loading