scripts: harden sftp.test, drop the external test - #1206
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the scripts/sftp.test POSIX sh test runner to reduce flakiness around server startup/ready-file handling and to improve portability and robustness of the shell code.
Changes:
- Reset the ready-file wait counter per
create_portinvocation and align the post-loop guard with the loop’s-scondition. - Remove non-POSIX
echo -eusage and apply safer shell practices (quoting,$(...),grep -q,exit 1in trap). - Simplify redundant checks/assignments in the test flow.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
ejohnstown
requested review from
wolfSSL-Fenrir-bot
and removed request for
wolfSSL-Fenrir-bot
August 27, 2026 23:25
ejohnstown
force-pushed
the
ccb-phase1-2
branch
from
August 28, 2026 19:42
cbd12ed to
afd4b20
Compare
ejohnstown
requested review from
wolfSSL-Fenrir-bot
and removed request for
wolfSSL-Fenrir-bot
August 28, 2026 19:43
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1206
No scan targets match the changed files in this PR. Review skipped.
The ready-file counter is shared by every create_port call and was never reset, so the twenty iterations are a budget for the whole run rather than per server start. A slow first start leaves later ones with no wait at all, and the test fails with "NO ready file" instead of waiting. The guard after the loop then disagreed with the loop as well: the wait is for a non-empty file, but the guard only asked whether the file existed, so a wait that ran out still went on to read an empty port. - reset the counter in create_port, so each server start gets the full wait and a new call site cannot forget it - test the guard on -s, matching what the loop waited for - drop "echo -e", undefined for POSIX sh and printed literally by shells that do not take the flag; the escapes were only ever blank lines - quote the expansions, use $(...) instead of backticks, and grep -q - exit 1 rather than exit -1 from the trap handler - drop the redundant PWD assignment before the set-directory test, and the second copy of the wolfsftp executable check Leaves shellcheck -s sh clean apart from SC2329 on the trap handler.
external.test connected the client and SFTP client to a host named by WOLFSSH_EXTERNAL_HOST. It only ran when WOLFSSH_EXTERNAL_TEST was set, which nothing in the tree or in CI does, so it always exited 77. - delete scripts/external.test - drop it from scripts/include.am
ejohnstown
force-pushed
the
ccb-phase1-2
branch
from
August 28, 2026 23:53
afd4b20 to
9868523
Compare
ejohnstown
changed the title
wolfSSL-Fenrir-bot
left a comment
wolfSSL-Fenrir-bot
left a comment
There was a problem hiding this comment.
Fenrir Automated Review — PR #1206
No scan targets match the changed files in this PR. Review skipped.
philljj
approved these changes
Aug 30, 2026
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
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.
sftp.testThe ready-file counter is shared by every
create_portcall and was never reset, so the twenty iterations are a budget for the whole run rather than per server start. A slow first start leaves later ones with no wait at all, and the test fails with "NO ready file" instead of waiting. The guard after the loop then disagreed with the loop as well: the wait is for a non-empty file, but the guard only asked whether the file existed, so a wait that ran out still went on to read an empty port.create_port, so each server start gets the full wait and a new call site cannot forget it-s, matching what the loop waited forecho -e, undefined for POSIX sh and printed literally by shells that do not take the flag; the escapes were only ever blank lines$(...)instead of backticks, andgrep -qexit 1rather thanexit -1from the trap handlerLeaves
shellcheck -s shclean apart from SC2329 on the trap handler.external.testexternal.testconnects the client and the SFTP client to a host named byWOLFSSH_EXTERNAL_HOST, and runs only whenWOLFSSH_EXTERNAL_TESTis set. Nothing in the tree or in CI sets either, so it has always exited 77.scripts/external.testand drop it fromscripts/include.am