NO-JIRA: Add project documentation to CLAUDE.md #8209
Conversation
Provides quick reference for running unit and subsystem tests including: - System requirements (Go, Docker/Podman, nmstate-devel, gotestsum) - Quick start commands for different test scenarios - Prerequisites check emphasizing gotestsum requirement - Running specific tests by package or test name - Environment variables reference - Troubleshooting common issues Make targets (unit-test, run-unit-test, ci-unit-test, subsystem-test) all require gotestsum. Tests will run for a long time before failing if gotestsum is not installed. Document this prominently to prevent users from wasting time. Assisted-by: Claude Code
openshift-ci-robot
added
the
jira/valid-reference
|
@zaneb: This pull request explicitly references no jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci
bot
added
the
size/XL
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: zaneb The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8209 +/- ##
===========================================
- Coverage 43.22% 23.09% -20.13%
===========================================
Files 404 275 -129
Lines 69935 42838 -27097
===========================================
- Hits 30226 9893 -20333
+ Misses 37003 32060 -4943
+ Partials 2706 885 -1821 🚀 New features to boost your workflow:
|
WalkthroughIntroduces a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.5.0)Error: can't load config: unsupported version of the configuration: "" See https://golangci-lint.run/docs/product/migration-guide for migration instructions 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
CLAUDE.md (5)
62-72: Use proper markdown heading instead of bold emphasis for "1. Check for container runtime".Lines 62 is flagged by markdownlint for using emphasis (
**...**) instead of a heading. Consider changing to a proper markdown heading (### 1. Check for container runtime) to improve document structure and pass linting.-**1. Check for container runtime (Docker or Podman)** +### 1. Check for container runtime (Docker or Podman)
78-92: Use proper markdown heading instead of bold emphasis for "2. Check for gotestsum".Line 78 is flagged by markdownlint for using emphasis instead of a heading. Consider changing to a proper markdown heading (
### 2. Check for gotestsum) for consistency and to pass linting.-**2. Check for gotestsum** +### 2. Check for gotestsum
154-156: Specify language identifier for code block.Line 154 has a fenced code block without a language identifier. Add
bashor an appropriate language tag.**Error:** -``` +```bash fatal error: nmstate.h: No such file or directory -``` +```
174-176: Specify language identifier for error output code block.Line 174 has a fenced code block without a language identifier. For error messages, use
textorbash.**Error:** -``` +```text Cannot connect to the Docker daemon at unix:///var/run/docker.sock -``` +```
348-351: Specify language identifier for error output code block.Line 348 has a fenced code block without a language identifier. For error/diagnostic output, use
textorbash.**Symptoms:** Tests fail in BeforeSuite with error message: -``` +```text failed to connect to `host=127.0.0.1 user=postgres database=`: dial tcp 127.0.0.1:5433: connect: connection refused -``` +```internal/bminventory/inventory_test.go (1)
373-375: Optional (Ginkgo v2): Use DeferCleanup to register teardown with the suite.If on Ginkgo v2, you can register cleanup right where you init:
var _ = BeforeSuite(func() { common.InitializeDBTest() DeferCleanup(common.TerminateDBTest) })This keeps lifecycle coupled and avoids a separate AfterSuite block. Use only if not running in parallel or combined with synchronized hooks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (14)
CLAUDE.md(1 hunks)internal/bminventory/inventory_test.go(1 hunks)internal/controller/controllers/controllers_suite_test.go(2 hunks)internal/events/event_test.go(1 hunks)internal/host/hostcommands/instruction_manager_test.go(1 hunks)internal/host/hostutil/host_utils_test.go(1 hunks)internal/infraenv/infraenv_suite_test.go(1 hunks)internal/manifests/manifests_test.go(1 hunks)internal/migrations/migrations_suite_test.go(1 hunks)internal/operators/handler/handler_suite_test.go(1 hunks)internal/releasesources/release_sources_test.go(1 hunks)internal/usage/manager_test.go(3 hunks)internal/versions/kube_api_versions_test.go(1 hunks)pkg/auth/auth_suite_test.go(1 hunks)
🧰 Additional context used
🪛 LanguageTool
CLAUDE.md
[grammar] ~438-~438: Use a hyphen to join words.
Context: ...assisted-test-infra) - Downstream: QE maintained tests ## Additional Resource...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
CLAUDE.md
62-62: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
78-78: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
154-154: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
174-174: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
348-348: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Red Hat Konflux / assisted-service-saas-main-on-pull-request
- GitHub Check: Red Hat Konflux / assisted-service-rhel9-acm-ds-main-on-pull-request
🔇 Additional comments (15)
CLAUDE.md (1)
1-420: Comprehensive testing guide—well-organized and valuable.The document is well-structured with clear sections, helpful examples, and practical troubleshooting. It effectively documents Claude's experience and provides a solid foundation for future test runs. The inclusion of unsandboxed execution notes for Claude Code, container runtime requirements, gotestsum dependencies, and database lifecycle management aligns well with the repository's shift to BeforeSuite/AfterSuite test hooks.
Minor markdown linting issues (noted in separate comments) can be fixed, but the content quality and comprehensiveness are strong.
internal/host/hostcommands/instruction_manager_test.go (1)
594-601: LGTM! Excellent refactoring of test infrastructure.The addition of suite-level database lifecycle hooks is well-implemented. This correctly separates global DB initialization (once per suite) from per-test DB creation (in BeforeEach), maintaining test isolation while improving efficiency.
internal/operators/handler/handler_suite_test.go (1)
15-22: LGTM! Consistent with suite-level DB lifecycle pattern.The suite hooks are correctly implemented and align with the broader refactoring across test files.
internal/events/event_test.go (1)
2015-2022: LGTM! Suite-level DB hooks properly added.The implementation correctly establishes suite-wide database lifecycle management.
internal/host/hostutil/host_utils_test.go (1)
640-647: LGTM! Suite-level initialization correctly added.The DB lifecycle hooks are properly implemented, matching the pattern established across the codebase.
internal/controller/controllers/controllers_suite_test.go (1)
48-63: LGTM! Suite hooks properly integrated with existing setup.The DB initialization is correctly added to the existing BeforeSuite hook, and the new AfterSuite ensures proper cleanup. Good integration with the pre-existing logger configuration.
internal/migrations/migrations_suite_test.go (1)
21-27: LGTM! Suite-level DB lifecycle correctly established.The hooks are properly implemented for migration tests.
internal/manifests/manifests_test.go (1)
36-42: LGTM! Suite initialization properly added.The DB lifecycle hooks are correctly implemented.
pkg/auth/auth_suite_test.go (1)
15-22: LGTM! Suite-level DB hooks correctly added.This completes the consistent refactoring of database lifecycle management across all test suites. The standardized approach will improve test reliability and maintainability.
internal/releasesources/release_sources_test.go (1)
21-27: LGTM! Clean suite-level DB lifecycle management.The addition of BeforeSuite/AfterSuite hooks consolidates database initialization at the suite level while maintaining per-test isolation through the existing BeforeEach/AfterEach hooks. This refactoring improves test organization and reduces redundant setup overhead.
internal/versions/kube_api_versions_test.go (1)
31-37: LGTM! Consistent suite-level setup.The BeforeSuite/AfterSuite hooks follow the same pattern as other test suites in this PR, centralizing database lifecycle management at the suite level.
internal/usage/manager_test.go (2)
23-29: LGTM! Suite-level DB initialization added.The BeforeSuite/AfterSuite hooks properly initialize and terminate the test database at the suite level, consistent with the refactoring pattern across other test files in this PR.
40-54: LGTM! Per-test isolation maintained.The BeforeEach/AfterEach hooks continue to create and clean up isolated database instances for each test, ensuring test independence while benefiting from the suite-level DB infrastructure setup.
internal/infraenv/infraenv_suite_test.go (1)
16-22: LGTM! Suite-level DB setup complete.The BeforeSuite/AfterSuite hooks properly establish database lifecycle management for the infraenv test suite, completing the consistent refactoring pattern across all test suites in this PR.
internal/bminventory/inventory_test.go (1)
373-379: Consider parallelism only if CI/tests enable it—currently disabled.The repo uses Ginkgo v1.16.5 and does not enable parallel test execution. The Makefile invokes tests without
-p,--nodes, or--parallelflags, soBeforeSuite/AfterSuiterun once as-is. The suggested refactor toSynchronizedBeforeSuite/AfterSuiteis optional and only necessary if you plan to enable parallel test execution (e.g., via future CI changes or developerginkgo -pinvocation). If parallelism is not a planned feature, the current hooks are safe.
Refactor 13 test suites to call InitializeDBTest() in Ginkgo BeforeSuite hooks instead of directly in Test functions. This follows the standard Ginkgo pattern and ensures proper error handling during suite setup. Benefits: - Ginkgo reports BeforeSuite failures with clear, actionable messages - Tests fail fast (~5s) when database is unavailable instead of timing out after 30s - Consistent pattern across all test suites Implementation: - Most files: Move from Test function to BeforeSuite hook with DeferCleanup for teardown - internal/controller/controllers: Merged DB init with existing logger setup in a single BeforeSuite - internal/usage: Changed nested BeforeSuite to BeforeEach (per-test setup belongs in BeforeEach, not BeforeSuite) Assisted-by: Claude Code
Add detailed sections on: - Project overview and architecture - Core components and package organization - Common development commands (building, code generation, linting) - Testing system requirements and dependencies - Running unit tests with make targets - Test categories and environment variables - Troubleshooting common issues - Advanced testing workflows Provide prerequisite checks and expected behavior when dependencies are unavailable. Assisted-by: Claude Code
The SKIP_UT_DB environment variable has a misleading name - it does not skip database-dependent tests. Instead, it tells the test framework to skip auto-creating a database and assume one is already running at localhost:5433. This commit updates CLAUDE.md to: - Explain the actual behavior of SKIP_UT_DB - Document the database creation fallback chain (K8s -> testcontainer) - Clarify that make unit-test uses SKIP_UT_DB after starting a container - Add guidance for environments without Docker/Podman - Document that many packages can be tested individually without database - Remove misleading claims that SKIP_UT_DB skips database tests Assisted-by: Claude Code
zaneb
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
CLAUDE.md (2)
98-565: Comprehensive testing documentation with minor formatting suggestions.The testing section is thorough and particularly valuable in clarifying the confusing
SKIP_UT_DBbehavior. The troubleshooting guidance is practical and covers common issues well.Consider these minor markdown formatting improvements:
1. Lines 146 & 166: Use proper headings instead of emphasis
-**1. Check for container runtime (Docker or Podman)** +#### 1. Check for container runtime (Docker or Podman) -**2. Check for gotestsum** +#### 2. Check for gotestsum2. Lines 421, 441, 455: Add language specifiers to code blocks
For error messages and console output, use
textorconsole:-``` +```text fatal error: nmstate.h: No such file or directory ``` -``` +```text Cannot connect to the Docker daemon at unix:///var/run/docker.sock ``` -``` +```text failed to connect to `host=127.0.0.1 user=postgres database=`: dial tcp 127.0.0.1:5433: connect: connection refused ```
567-643: LGTM! Helpful deployment and patterns guidance.The deployment scenarios and key patterns sections provide valuable context for understanding the codebase structure and conventions.
Consider adding a hyphen on line 369 for clarity:
- - Downstream: QE maintained tests + - Downstream: QE-maintained tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (16)
CLAUDE.md(1 hunks)internal/bminventory/inventory_test.go(1 hunks)internal/cluster/cluster_suite_test.go(1 hunks)internal/controller/controllers/controllers_suite_test.go(1 hunks)internal/events/event_test.go(1 hunks)internal/host/hostcommands/instruction_manager_test.go(1 hunks)internal/host/hosts_suite_test.go(1 hunks)internal/host/hostutil/host_utils_test.go(1 hunks)internal/infraenv/infraenv_suite_test.go(1 hunks)internal/manifests/manifests_test.go(1 hunks)internal/migrations/migrations_suite_test.go(1 hunks)internal/operators/handler/handler_suite_test.go(1 hunks)internal/releasesources/release_sources_test.go(1 hunks)internal/usage/manager_test.go(3 hunks)internal/versions/kube_api_versions_test.go(1 hunks)pkg/auth/auth_suite_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/versions/kube_api_versions_test.go
- internal/releasesources/release_sources_test.go
- internal/events/event_test.go
- internal/manifests/manifests_test.go
🧰 Additional context used
🪛 LanguageTool
CLAUDE.md
[style] ~220-~220: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ...se Management (For Development) If you want to run tests repeatedly without restarting...
(REP_WANT_TO_VB)
[grammar] ~369-~369: Use a hyphen to join words.
Context: ...assisted-test-infra) - Downstream: QE maintained tests ### Environment Variab...
(QB_NEW_EN_HYPHEN)
🪛 markdownlint-cli2 (0.18.1)
CLAUDE.md
146-146: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
166-166: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
421-421: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
441-441: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
455-455: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (14)
internal/operators/handler/handler_suite_test.go (1)
16-19: LGTM! Suite-level DB lifecycle management.This refactor consolidates database initialization and cleanup at the suite level, improving test organization and maintainability.
internal/host/hostcommands/instruction_manager_test.go (1)
595-598: LGTM! Consistent suite-level DB setup.The addition of suite-level database lifecycle management aligns with the broader test refactor across the codebase.
internal/controller/controllers/controllers_suite_test.go (1)
49-50: LGTM! DB lifecycle integrated into existing suite setup.The database initialization and cleanup are appropriately added to the existing BeforeSuite, ensuring proper sequencing with the logger configuration.
internal/host/hosts_suite_test.go (1)
29-29: LGTM! Cleanup migration to DeferCleanup.Replacing AfterSuite-based termination with DeferCleanup aligns with Ginkgo v2 best practices and ensures proper cleanup ordering.
internal/migrations/migrations_suite_test.go (1)
21-24: LGTM! Suite-level DB lifecycle management.This change consolidates database initialization and termination at the suite level, consistent with the broader refactor across test suites.
internal/cluster/cluster_suite_test.go (1)
29-29: LGTM! Cleanup migration to DeferCleanup.This completes the migration from AfterSuite to DeferCleanup, following Ginkgo v2 best practices.
internal/host/hostutil/host_utils_test.go (1)
641-644: LGTM! Suite-level DB setup added.The addition of suite-level database lifecycle management is consistent with the test infrastructure improvements across the codebase.
pkg/auth/auth_suite_test.go (1)
16-19: LGTM! Suite-level DB lifecycle management.This final addition completes the consistent refactor of database lifecycle management across all test suites in the PR.
internal/usage/manager_test.go (2)
23-26: LGTM! Suite-level DB initialization.The refactor moves database initialization to BeforeSuite with proper cleanup via DeferCleanup. This is more efficient than per-test initialization and consistent with the pattern adopted across the codebase in this PR.
37-51: LGTM! Per-test DB lifecycle retained.The BeforeEach/AfterEach hooks correctly maintain per-test database preparation and cleanup via PrepareTestDB/DeleteTestDB, complementing the suite-level initialization.
internal/infraenv/infraenv_suite_test.go (1)
16-19: LGTM! Suite-level DB initialization.The BeforeSuite hook properly initializes the database for the entire test suite and registers cleanup via DeferCleanup. This is consistent with the pattern applied across the codebase in this PR.
CLAUDE.md (2)
1-52: LGTM! Clear project overview and architecture.The introduction effectively explains the dual-mode nature of the service (REST API and Kube-API) and provides a well-organized overview of core components and package organization.
644-672: LGTM! Good references and resources.The closing sections provide helpful pointers to additional documentation and important project files.
internal/bminventory/inventory_test.go (1)
373-376: Need to verify the actual helper function implementations and check the go.mod to understand the intended Ginkgo version:Based on my web search and script verification, I have found a critical issue: DeferCleanup is a Ginkgo v2 function, but the file imports Ginkgo v1 (. "github.com/onsi/ginkgo"). This is a compile-time error. The original review comment is fundamentally incorrect for this codebase and must be rejected.Review comment is incorrect and must be ignored.
The code snippet uses
DeferCleanup(common.TerminateDBTest), which is a Ginkgo v2 function. However, the test file importsgithub.com/onsi/ginkgo(v1), notgithub.com/onsi/ginkgo/v2. V1 is no longer supported, and DeferCleanup does not exist in v1—this code will not compile as written.Either:
- The file needs to be migrated to Ginkgo v2 imports (
. "github.com/onsi/ginkgo/v2"), or- This code change is incomplete/incorrect and should not be merged.
The original review comment's suggestions about synchronized hooks and parallel semantics only apply if Ginkgo v2 is adopted. Without confirming the actual Ginkgo version in the codebase, no other guidance is valid.
Likely an incorrect or invalid review comment.
|
/retest-required |
|
@zaneb: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
2 participants
Record Claude's experience in running the unit tests and discovering the project structure to CLAUDE.md so that in future it will not have to rediscover any of it from first principles.