Skip to content

test: add tests for KORTEX_CLI_STORAGE support#26

Merged
feloy merged 1 commit intokortex-hub:mainfrom
feloy:storage-env-var
Mar 9, 2026
Merged

test: add tests for KORTEX_CLI_STORAGE support#26
feloy merged 1 commit intokortex-hub:mainfrom
feloy:storage-env-var

Conversation

@feloy
Copy link
Contributor

@feloy feloy commented Mar 9, 2026

No description provided.

Signed-off-by: Philippe Martin <phmartin@redhat.com>
@coderabbitai
Copy link

coderabbitai bot commented Mar 9, 2026

📝 Walkthrough

Walkthrough

This PR adds documentation updates to AGENTS.md clarifying testing practices with environment variables and parallel tests, and introduces a new test suite TestRootCmd_StorageEnvVariable in pkg/cmd/root_test.go that validates environment variable and flag override behavior for the storage configuration.

Changes

Cohort / File(s) Summary
Documentation
AGENTS.md
Added exception note for t.Setenv() usage with parallel tests; updated guidance to prefer fake implementations over mocks in interface-based testing with concrete examples.
Testing
pkg/cmd/root_test.go
Introduced new test suite TestRootCmd_StorageEnvVariable with three subtests validating environment variable defaults, flag overrides, and fallback behavior for the storage configuration.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ❓ Inconclusive No description was provided by the author, making it impossible to assess relevance to the changeset. Add a brief description explaining what the tests validate and why KORTEX_CLI_STORAGE support is important.
✅ Passed checks (1 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change: adding tests for KORTEX_CLI_STORAGE environment variable support, which matches the test suite added in root_test.go.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
pkg/cmd/root_test.go (1)

211-213: Minor: Comment could be more precise.

The comment says "unset the environment variable" but t.Setenv("KORTEX_CLI_STORAGE", "") sets it to an empty string rather than truly unsetting it. This works correctly here because NewRootCmd() checks envStorage != "", but the distinction could matter in other contexts.

📝 Suggested comment clarification
 	t.Run("default used when env var not set", func(t *testing.T) {
-		// Explicitly unset the environment variable (in case it was set in the shell)
+		// Set the environment variable to empty string to simulate it not being set
 		t.Setenv("KORTEX_CLI_STORAGE", "")
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pkg/cmd/root_test.go` around lines 211 - 213, The comment in the test around
t.Setenv("KORTEX_CLI_STORAGE", "") is imprecise: t.Setenv sets the variable to
an empty string rather than unsetting it. Update the comment to state that the
env var is being set to an empty string (or, if you intend to truly unset it,
replace t.Setenv with a call that unsets the variable such as os.Unsetenv or
t.Setenv(..., "") paired with removing the key in the environment) and ensure
the test behavior around NewRootCmd() (which checks envStorage != "") is clearly
referenced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@pkg/cmd/root_test.go`:
- Around line 211-213: The comment in the test around
t.Setenv("KORTEX_CLI_STORAGE", "") is imprecise: t.Setenv sets the variable to
an empty string rather than unsetting it. Update the comment to state that the
env var is being set to an empty string (or, if you intend to truly unset it,
replace t.Setenv with a call that unsets the variable such as os.Unsetenv or
t.Setenv(..., "") paired with removing the key in the environment) and ensure
the test behavior around NewRootCmd() (which checks envStorage != "") is clearly
referenced.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b82789eb-2f7d-41da-a8c2-03dcf24d4be5

📥 Commits

Reviewing files that changed from the base of the PR and between d9b2fd5 and 5379f42.

📒 Files selected for processing (2)
  • AGENTS.md
  • pkg/cmd/root_test.go

@feloy feloy requested review from benoitf and jeffmaury March 9, 2026 10:23
@feloy feloy merged commit 2bcefa0 into kortex-hub:main Mar 9, 2026
6 checks passed
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