Skip to content

test_tekton_tasks: fix TEST_ITEMS emptiness check#68

Merged
chmeliik merged 1 commit into
konflux-ci:mainfrom
chmeliik:fix-test-tekton-tasks-args-check
Jul 22, 2026
Merged

test_tekton_tasks: fix TEST_ITEMS emptiness check#68
chmeliik merged 1 commit into
konflux-ci:mainfrom
chmeliik:fix-test-tekton-tasks-args-check

Conversation

@chmeliik

Copy link
Copy Markdown
Collaborator

Previously, the check would consider a whitespace-only TEST_ITEMS env var valid input but wouldn't execute any tests.

Fix by turning the TEST_ITEMS string into an array before checking the emptiness.

Previously, the check would consider a whitespace-only TEST_ITEMS env
var valid input but wouldn't execute any tests.

Fix by turning the TEST_ITEMS string into an array *before* checking the
emptiness.

Signed-off-by: Adam Cmiel <acmiel@redhat.com>
@chmeliik
chmeliik requested a review from a team as a code owner July 22, 2026 13:00
@gemini-code-assist

Copy link
Copy Markdown

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@qodo-app-for-konflux-ci

Copy link
Copy Markdown

PR Summary by Qodo

Fix whitespace-only TEST_ITEMS handling in test_tekton_tasks script

🐞 Bug fix 🕐 10-20 Minutes

Grey Divider

AI Description

• Parse TEST_ITEMS into an array before validating it.
• Treat whitespace-only TEST_ITEMS as empty and fail fast with usage output.
• Keep CLI args and env var inputs consistent across generated script templates.
Diagram

graph TD
  A["Invoke script"] --> B["Build TEST_ITEMS[]"] --> C{"-h or empty?"}
  C -->|"yes"| D["Print usage + exit"]
  C -->|"no"| E["Validate items"] --> F["Execute tests"]
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Whitespace-trim TEST_ITEMS string before the existing -z check
  • ➕ Minimal diff (could avoid moving code blocks).
  • ➕ Keeps the initial help/empty guard based on a string check.
  • ➖ Duplicates logic: still need array parsing later, risking future drift.
  • ➖ Harder to ensure the guard matches the real execution input (the array).
2. Regex validate env var before parsing (e.g., require a non-space character)
  • ➕ Makes the acceptance criteria explicit (must contain a non-whitespace token).
  • ➖ Still separate from the array actually consumed by the loop.
  • ➖ More Bash-specific nuance (regex/locale/IFS) for little benefit.

Recommendation: The PR’s approach is best: normalize both CLI args and env var into TEST_ITEMS[] first, then validate based on array length. This guarantees the emptiness check matches what the script will actually iterate over and prevents whitespace-only env vars from silently skipping all tests.

Files changed (2) +16 / -16

Bug fix (2) +16 / -16
test_tekton_tasks.shParse TEST_ITEMS early and validate via array length +8/-8

Parse TEST_ITEMS early and validate via array length

• Moves the TEST_ITEMS construction (from args or env) ahead of the usage/emptiness check. The script now checks '${#TEST_ITEMS[@]}' so whitespace-only env input is treated as empty and fails fast.

.github/scripts/test_tekton_tasks.sh

test_tekton_tasks.shMirror TEST_ITEMS array-based emptiness validation in template +8/-8

Mirror TEST_ITEMS array-based emptiness validation in template

• Applies the same early parsing and array-length emptiness check to the cookiecutter-templated version of the script to keep generated repos consistent.

{{cookiecutter.repo_root}}/.github/scripts/test_tekton_tasks.sh

@qodo-app-for-konflux-ci

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Grey Divider

Qodo Logo

@chmeliik
chmeliik merged commit e3e9023 into konflux-ci:main Jul 22, 2026
10 checks passed
@chmeliik
chmeliik deleted the fix-test-tekton-tasks-args-check branch July 22, 2026 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants