🧪 testing improvement description#51
Conversation
- Creates `test/test-slugify.sh` with comprehensive test coverage including standard strings, empty strings, multi-hyphen behaviors, special characters, unicode strings, implicit/explicit standard inputs, and terminal detections. - Replaces scattered inline slugify assertions in `test/smoke-test.sh` with a call to the new test script to reduce redundancy. Co-authored-by: savvides <1580637+savvides@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request extracts the inline slugify tests from test/smoke-test.sh into a dedicated unit test suite in test/test-slugify.sh, expanding test coverage to include various edge cases, unicode handling, and input methods. Feedback on the new test script includes resolving a Python compatibility issue with os.waitstatus_to_exitcode on older Python versions, using pwd -P to handle symbolic links robustly, and avoiding toggling set -e globally inside a function by using an OR list to capture exit codes.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| # The tool checks [ -t 0 ] to detect if it's connected to a terminal. | ||
| # We simulate a terminal using python's pty module. | ||
| if command -v python3 >/dev/null 2>&1; then | ||
| assert_exit_code "no input given (exit 3)" "python3 -c 'import sys, os, pty, subprocess; pid, fd = pty.fork(); sys.exit(subprocess.call([\"$SLUGIFY\"])) if pid == 0 else sys.exit(os.waitstatus_to_exitcode(os.waitpid(pid, 0)[1]))'" 3 |
There was a problem hiding this comment.
The python command uses os.waitstatus_to_exitcode, which was introduced in Python 3.9. This will cause the test suite to fail with an AttributeError on systems running older Python versions (such as Python 3.8, which is the default on Ubuntu 20.04 LTS).
Using os.execv and os.WEXITSTATUS is fully compatible with all Python 3 versions and is cleaner.
| assert_exit_code "no input given (exit 3)" "python3 -c 'import sys, os, pty, subprocess; pid, fd = pty.fork(); sys.exit(subprocess.call([\"$SLUGIFY\"])) if pid == 0 else sys.exit(os.waitstatus_to_exitcode(os.waitpid(pid, 0)[1]))'" 3 | |
| assert_exit_code "no input given (exit 3)" "python3 -c 'import os, pty, sys; pid, fd = pty.fork(); os.execv(\"$SLUGIFY\", [\"$SLUGIFY\"]) if pid == 0 else sys.exit(os.WEXITSTATUS(os.waitpid(pid, 0)[1]))'" 3 |
| FAIL=0 | ||
| TOTAL=0 | ||
|
|
||
| REPO_ROOT="$(cd "$(dirname "$0")/.." && pwd)" |
There was a problem hiding this comment.
To ensure robustness when the repository or test scripts are accessed via symbolic links, it is recommended to use pwd -P instead of pwd. This resolves the physical directory structure and maintains consistency with how IDSTACK_DIR is resolved in test/smoke-test.sh.
| REPO_ROOT="$(cd "$(dirname "$0")/.." && pwd)" | |
| REPO_ROOT="$(cd "$(dirname "$0")/.." && pwd -P)" |
| set +e | ||
| eval "$command" >/dev/null 2>&1 | ||
| local code=$? | ||
| set -e |
There was a problem hiding this comment.
Toggling set -e and set +e globally within a function can be risky if the function exits prematurely or if there are other side effects. Instead, you can safely capture the exit code of the command without triggering set -e by executing it as part of an || (OR) list.
| set +e | |
| eval "$command" >/dev/null 2>&1 | |
| local code=$? | |
| set -e | |
| local code=0 | |
| eval "$command" >/dev/null 2>&1 || code=$? |
1 participant
🎯 What: Adds missing unit tests for the string manipulation logic in
bin/idstack-slugify.📊 Coverage: The new test script (
test/test-slugify.sh) handles edge cases (leading/trailing/multiple hyphens), special characters, NFKD-folding of Unicode/non-ASCII characters, explicit and implicit standard input pipelining, empty strings, and absence-of-TTY behaviors.✨ Result: Test assertions are centralized, and
idstack-slugifypure string functions are thoroughly validated, increasing codebase test coverage.PR created automatically by Jules for task 6863284810201051258 started by @savvides