Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
128 changes: 128 additions & 0 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,134 @@ The test suite relies on the `setup-envtest` tooling from `sigs.k8s.io/controlle
The first run downloads a Kubernetes `envtest` environment from the internet, so network access is required.
Without it some tests will fail during setup.

### Testing Patterns and Guidelines

This project follows specific testing patterns to ensure consistency, maintainability, and quality. When writing tests, adhere to the following guidelines:

#### Test Framework

- **Use `testify/suite`** for organizing tests into suites
- Tests should be structured using test suites that embed `suite.Suite`
- Each test file should have a corresponding suite struct (e.g., `UnstructuredSuite`, `KubevirtSuite`)
- Use the `suite.Run()` function to execute test suites

Example:
```go
type MyTestSuite struct {
suite.Suite
}

func (s *MyTestSuite) TestSomething() {
s.Run("descriptive scenario name", func() {
// test implementation
})
}

func TestMyFeature(t *testing.T) {
suite.Run(t, new(MyTestSuite))
}
```

#### Behavior-Based Testing

- **Test the public API only** - tests should be black-box and not access internal/private functions
- **No mocks** - use real implementations and integration testing where possible
- **Behavior over implementation** - test what the code does, not how it does it
- Focus on observable behavior and outcomes rather than internal state

#### Test Organization

- **Use nested subtests** with `s.Run()` to organize related test cases
- **Descriptive names** - subtest names should clearly describe the scenario being tested
- Group related scenarios together under a parent test (e.g., "edge cases", "with valid input")

Example structure:
```go
func (s *MySuite) TestFeature() {
s.Run("valid input scenarios", func() {
s.Run("handles simple case correctly", func() {
// test code
})
s.Run("handles complex case with nested data", func() {
// test code
})
})
s.Run("edge cases", func() {
s.Run("returns error for nil input", func() {
// test code
})
s.Run("handles empty input gracefully", func() {
// test code
})
})
}
```

#### Assertions

- **One assertion per test case** - each `s.Run()` block should ideally test one specific behavior
- Use `testify` assertion methods: `s.Equal()`, `s.True()`, `s.False()`, `s.Nil()`, `s.NotNil()`, etc.
- Provide clear assertion messages when the failure reason might not be obvious

Example:
```go
s.Run("returns expected value", func() {
result := functionUnderTest()
s.Equal("expected", result, "function should return the expected string")
})
```

#### Coverage

- **Aim for high test coverage** of the public API
- Add edge case tests to cover error paths and boundary conditions
- Common edge cases to consider:
- Nil/null inputs
- Empty strings, slices, maps
- Negative numbers or invalid indices
- Type mismatches
- Malformed input (e.g., invalid paths, formats)

#### Error Handling

- **Never ignore errors** in production code
- Always check and handle errors from functions that return them
- In tests, use `s.Require().NoError(err)` for operations that must succeed for the test to be valid
- Use `s.Error(err)` or `s.NoError(err)` for testing error conditions

Example:
```go
s.Run("returns error for invalid input", func() {
result, err := functionUnderTest(invalidInput)
s.Error(err, "expected error for invalid input")
s.Nil(result, "result should be nil when error occurs")
})
```

#### Test Helpers

- Create reusable test helpers in `internal/test/` for common testing utilities
- Test helpers should be generic and reusable across multiple test files
- Document test helpers with clear godoc comments explaining their purpose and usage

Example from this project:
```go
// FieldString retrieves a string field from an unstructured object using JSONPath-like notation.
// Examples:
// - "spec.runStrategy"
// - "spec.template.spec.volumes[0].containerDisk.image"
func FieldString(obj *unstructured.Unstructured, path string) string {
// implementation
}
```

#### Examples from the Codebase

Good examples of these patterns can be found in:
- `internal/test/unstructured_test.go` - demonstrates proper use of testify/suite, nested subtests, and edge case testing
- `pkg/mcp/kubevirt_test.go` - shows behavior-based testing of the MCP layer
- `pkg/kubernetes/manager_test.go` - illustrates testing with proper setup/teardown and subtests

## Linting

Static analysis is performed with `golangci-lint`:
Expand Down
172 changes: 172 additions & 0 deletions internal/test/unstructured.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,172 @@
// Package test provides utilities for testing unstructured Kubernetes objects.
//
// The primary functionality is JSONPath-like field access for unstructured.Unstructured objects,
// making test assertions more readable and maintainable.
package test

import (
"fmt"

"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
)

// FieldString retrieves a string field from an unstructured object using JSONPath-like notation.
// Examples:
// - "spec.runStrategy"
// - "spec.template.spec.volumes[0].containerDisk.image"
// - "spec.dataVolumeTemplates[0].spec.sourceRef.kind"
Comment on lines +13 to +17
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same as my comment about FieldInt - it would be nice to be able to tell the difference between not set and set to ""

Copy link
Member Author

Choose a reason for hiding this comment

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

For that purpose you would use test.FieldExists(vm, "spec.instancetype") and expect it to be false

https://github.com/marcnuri-forks/kubernetes-mcp-server/blob/8e5f6df7d009ebb7785c12d46faed9e73048be23/pkg/mcp/kubevirt_test.go#L283

func FieldString(obj *unstructured.Unstructured, path string) string {
if obj == nil {
return ""
}
value, _ := Field(obj.Object, path)
if str, ok := value.(string); ok {
return str
}
return ""
}

// FieldExists checks if a field exists at the given JSONPath-like path.
func FieldExists(obj *unstructured.Unstructured, path string) bool {
if obj == nil {
return false
}
_, found := Field(obj.Object, path)
return found
}

// FieldInt retrieves an integer field from an unstructured object using JSONPath-like notation.
// Returns 0 if the field is not found or is not an integer type (int, int64, int32).
Copy link
Collaborator

Choose a reason for hiding this comment

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

@manusa I'm not 100% sure this behaviour is ideal - there could be scenarios where I'm testing and expecting to receive a value of 0 - for example status.replicas. How would we be able to distinguish between the value actually being 0 vs. not being set/not being an int?

Or do you think that's fine because this is for tests? In that case I'm fine with it but let's add a note in the docstring about ensuring that you don't assert expecting an actual value to be 0

Copy link
Member Author

Choose a reason for hiding this comment

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

For that purpose you would use test.FieldExists(vm, "spec.instancetype") and expect it to be false

https://github.com/marcnuri-forks/kubernetes-mcp-server/blob/8e5f6df7d009ebb7785c12d46faed9e73048be23/pkg/mcp/kubevirt_test.go#L283

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll need to revisit for this case for the expecting to be 0

// Examples:
// - "spec.replicas"
// - "spec.ports[0].containerPort"
func FieldInt(obj *unstructured.Unstructured, path string) int64 {
if obj == nil {
return 0
}
value, _ := Field(obj.Object, path)
switch v := value.(type) {
case int64:
return v
case int:
return int64(v)
case int32:
return int64(v)
default:
return 0
}
}

// FieldValue retrieves any field value from an unstructured object using JSONPath-like notation.
// Returns nil if the field is not found. This is useful when you need the raw value
// without type conversion.
// Examples:
// - "spec.template.spec.containers[0]" - returns map[string]interface{}
// - "metadata.labels" - returns map[string]interface{}
func FieldValue(obj *unstructured.Unstructured, path string) interface{} {
if obj == nil {
return nil
}
value, _ := Field(obj.Object, path)
return value
}

// Field is the core helper that traverses an unstructured object using JSONPath-like notation.
// It supports both dot notation (foo.bar) and array indexing (foo[0].bar).
func Field(obj interface{}, path string) (interface{}, bool) {
if obj == nil || path == "" {
return nil, false
}

// Parse the path into segments
segments := parsePath(path)
current := obj

for _, segment := range segments {
if segment.isArray {
// Handle array indexing
slice, ok := current.([]interface{})
if !ok {
return nil, false
}
if segment.index >= len(slice) || segment.index < 0 {
return nil, false
}
current = slice[segment.index]
} else {
// Handle map field access
m, ok := current.(map[string]interface{})
if !ok {
return nil, false
}
val, exists := m[segment.field]
if !exists {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@manusa I think you still need a nil check on val here as well

return nil, false
}
current = val
}
}

return current, true
}

type pathSegment struct {
field string
isArray bool
index int
}

// parsePath converts a JSONPath-like string into segments.
// Examples:
// - "spec.runStrategy" -> [{field: "spec"}, {field: "runStrategy"}]
// - "spec.volumes[0].name" -> [{field: "spec"}, {field: "volumes"}, {isArray: true, index: 0}, {field: "name"}]
func parsePath(path string) []pathSegment {
var segments []pathSegment
current := ""
inBracket := false
indexStr := ""

for i := 0; i < len(path); i++ {
ch := path[i]
switch ch {
case '.':
if inBracket {
indexStr += string(ch)
} else if current != "" {
segments = append(segments, pathSegment{field: current})
current = ""
}
case '[':
if current != "" {
segments = append(segments, pathSegment{field: current})
current = ""
}
inBracket = true
indexStr = ""
case ']':
if inBracket {
// Parse the index
var idx int
if _, err := fmt.Sscanf(indexStr, "%d", &idx); err != nil {
// If parsing fails, use -1 as invalid index
idx = -1
}
segments = append(segments, pathSegment{isArray: true, index: idx})
inBracket = false
indexStr = ""
}
default:
if inBracket {
indexStr += string(ch)
} else {
current += string(ch)
}
}
}

if current != "" {
segments = append(segments, pathSegment{field: current})
}

return segments
}
Loading