Skip to content

fix: buildtest --no-benchmarks/--no-integration flag typos and broken TEST_INTEGRATION assignment#726

Merged
dnyw4l3n13 merged 3 commits into
mainfrom
fix/679-buildtest-flag-typos
Jul 20, 2026
Merged

fix: buildtest --no-benchmarks/--no-integration flag typos and broken TEST_INTEGRATION assignment#726
dnyw4l3n13 merged 3 commits into
mainfrom
fix/679-buildtest-flag-typos

Conversation

@dnyw4l3n13

Copy link
Copy Markdown
Collaborator

Summary

Scaffolding PR for #679. The original OOM root cause (--parallel-algorithm aggressive racing benchmark assemblies against unit tests) is already fixed on main by #723's commit 6e022f5: the main dotnet test pass now always excludes every benchmark test project, and benchmark projects (if any) are run individually afterwards.

Remaining scope for this PR

In development/buildtest:

  1. Fix the flag typos:

    • -b|--no-benchmmarks becomes -b|--no-benchmarks (line 69, plus the matching comment reference at line ~165)
    • -i|--no-integratopn becomes -i|--no-integration (line 73)
  2. Fix the still-broken TEST_INTEGRATION assignment (line 74):

    TEST_INTEGRATION=--filter-not-namespace ".Integration.Tests" --filter-not-namespace ".Integration.Tests.*"

    This is parsed by sh as an env-var-prefixed command invocation (*.Integration.Tests as a command name), not a simple assignment, so --no-integration/-i is currently a silent no-op. Suggested fix, mirroring the style already used for the (now-fixed) TEST_BENCHMARKS variable and the existing "shellcheck disable=SC2086" on the dotnet test invocation that consumes it unquoted:

    TEST_INTEGRATION="--filter-not-namespace *.Integration.Tests --filter-not-namespace .Integration.Tests."

    No open PR/caller in this repo passes -b/-i today (confirmed via search), so renaming the flags is safe.

Verified: shellcheck development/buildtest is clean both before and after this class of change; no .csproj/.sln in this repo (shell-scripts-only, so no dotnet buildcheck applies).

Out of scope / needs follow-up

credfeto asked (issue comment, 2026-07-19) whether --parallel-algorithm sequential (and --long-running) could be reintroduced for the per-project benchmark dotnet test invocation, ideally as part of this issue's scope. This can't be verified from within credfeto/scripts itself - there are no benchmark test projects here - and prior direct evidence (#723's issue body, commit a0bf8ee) showed at least one real benchmark project's test host rejects those flags outright (exit code 5, "Zero tests ran") when run standalone. Recommend validating this against a real benchmark project (e.g. in funfair-server-ethereum) before deciding whether to add it back; flagging here rather than silently narrowing scope.

Test strategy

  • shellcheck development/buildtest after the fix
  • Manual inspection that $TEST_INTEGRATION word-splits into exactly the four intended arguments when passed unquoted to dotnet test

Closes #679

Reserves the changelog entry for the development/buildtest fix ahead
of implementation: the --no-benchmmarks/--no-integratopn flag typos
and the broken TEST_INTEGRATION filter assignment.

Prompt: Work on issue #679 in credfeto/scripts.
@dnyw4l3n13 dnyw4l3n13 added the AI-Work Work for an AI Agent label Jul 20, 2026
…d TEST_INTEGRATION assignment

The --no-benchmmarks and --no-integratopn option names were misspelled so
neither flag could ever be passed on the command line, and the
TEST_INTEGRATION assignment split into a bare variable assignment followed
by an attempt to execute "*.Integration.Tests" as a command instead of
capturing the filter arguments.

Fixes #679
@dnyw4l3n13

Copy link
Copy Markdown
Collaborator Author

Fixed in 2759ad0 - corrected the --no-benchmmarks/--no-integratopn flag-name typos in development/buildtest (now --no-benchmarks/--no-integration) and fixed the broken TEST_INTEGRATION assignment that was splitting into a bare variable assignment plus an attempted command execution of *.Integration.Tests; the branch previously only contained the changelog placeholder. Pushed and moved board status to Development.

@dnyw4l3n13

Copy link
Copy Markdown
Collaborator Author

Simplify clean - advancing to code review

@dnyw4l3n13

Copy link
Copy Markdown
Collaborator Author

Code Review: buildtest flag typo fix (#679)

Summary

Small, well-scoped fix: corrects two case pattern typos (--no-benchmmarks -> --no-benchmarks, --no-integratopn -> --no-integration) and repairs the TEST_INTEGRATION assignment, which previously wasn't quoted and would have been parsed by the shell as a variable assignment followed by an attempt to execute a command named after the glob-expansion of *.Integration.Tests. Low blast radius (2 files, one local dev script), but one correctness issue in the fix itself is worth addressing.

Warnings (should fix, creates tech debt)

  • Unquoted $TEST_INTEGRATION expansion is still subject to pathname (glob) expansion - development/buildtest:185
    TEST_INTEGRATION is now safely assigned as a single quoted string (good - closes the command-injection-via-glob risk from the original typo'd code), but it's expanded unquoted at the dotnet test call site to get word-splitting across the two --filter-not-namespace args. That same unquoted expansion also triggers pathname expansion: since cd "$SOURCE_DIR" (line 106) puts the shell in the solution root before this runs, any *.Integration.Tests / *.Integration.Tests.*-matching file sitting directly in that directory (e.g. a flat-layout Foo.Integration.Tests.csproj) would have its glob silently replaced by matching filenames instead of being passed through literally to dotnet test, corrupting the very filter --no-integration is supposed to apply. The sibling benchmark filters on lines 181-184 avoid this because they're quoted literals at the call site, not an unquoted variable holding glob metacharacters.
    Suggest wrapping the dotnet test invocation with set -f / set +f (POSIX-portable) to disable pathname expansion while keeping word-splitting, so the literal * reaches dotnet test's own filter syntax unmangled regardless of $SOURCE_DIR's contents.

Architecture Compliance

N/A - standalone shell script, no layering.

Test Coverage

No bats tests exist for buildtest (none did before this change either). Given the fix is a couple of lines and manually verified against shellcheck, adding coverage is optional here, not blocking.

What's Good

  • Root cause correctly diagnosed and documented in the CHANGELOG entry.
  • Comment updates (line 165) kept in sync with the renamed flag.
  • shellcheck reports no new issues.

…n filter

The TEST_INTEGRATION variable is expanded unquoted at the dotnet test
call site to get word-splitting across the two --filter-not-namespace
arguments. That same unquoted expansion also triggers shell pathname
expansion: since the script cd's into the solution root before this
runs, a *.Integration.Tests(.*)-matching file there would have its
glob silently replaced by matching filenames instead of the literal
filter, corrupting --no-integration. Wrap the dotnet test invocation
with set -f/set +f (POSIX-portable) to disable globbing while keeping
word-splitting.

Prompt: Work on pull request #726 in credfeto/scripts.
The repository is checked out at /workspace/repo.
@dnyw4l3n13

Copy link
Copy Markdown
Collaborator Author

Code review round: fixed 1 finding in 3c5866d - guarded the dotnet test invocation with set -f/set +f so an unquoted $TEST_INTEGRATION glob pattern can't be pathname-expanded against files in the solution directory.

@dnyw4l3n13

Copy link
Copy Markdown
Collaborator Author

Code Review: buildtest flag typo fix (#679) - round 2

Summary

Reviewed the current diff (main...HEAD), including the set -f/set +f fix from the previous round. No new findings.

Verification

  • shellcheck development/buildtest - clean, no warnings.
  • The set -f guard correctly wraps the entire dotnet test invocation that references the unquoted $TEST_INTEGRATION, and set +f restores pathname expansion immediately after - this closes the glob-corruption risk regardless of what files exist in $SOURCE_DIR.
  • No stale references to the old typo'd flag names (--no-benchmmarks, --no-integratopn) remain anywhere in the repo.
  • Comment at line 165 and CHANGELOG entries accurately describe both fixes.

What's Good

  • Root cause (unquoted glob-prone variable) fully closed, not just papered over.
  • Fix is minimal and scoped to the exact call site that needed it.

No findings - advancing to security review.

@dnyw4l3n13

Copy link
Copy Markdown
Collaborator Author

Security review clean - advancing to finalize

@dnyw4l3n13
dnyw4l3n13 marked this pull request as ready for review July 20, 2026 08:09
@dnyw4l3n13
dnyw4l3n13 enabled auto-merge July 20, 2026 08:09
@dnyw4l3n13

Copy link
Copy Markdown
Collaborator Author

PR was in draft state; marked ready for review and auto-merge (merge commit) enabled. Ready to merge once required review/checks complete.

@credfeto

Copy link
Copy Markdown
Owner

Super-linter summary

Language Validation result
BASH Pass ✅

All files and directories linted successfully

For more information, see the GitHub Actions workflow run

Powered by Super-linter

@dnyw4l3n13
dnyw4l3n13 merged commit 35681bc into main Jul 20, 2026
39 checks passed
@dnyw4l3n13
dnyw4l3n13 deleted the fix/679-buildtest-flag-typos branch July 20, 2026 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI-Work Work for an AI Agent

Projects

None yet

Development

Successfully merging this pull request may close these issues.

buildtest: aggressive parallel algorithm causes OOM when benchmark tests run

2 participants