test(entities/models): add unit tests for EntityInstance and EntityWi…#6245
test(entities/models): add unit tests for EntityInstance and EntityWi…#6245dakshhhhh16 wants to merge 1 commit intomindersec:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new unit test file for internal/entities/models, covering EntityInstance / EntityWithProperties string formatting, property update behavior, and the NeedsPropertyLoad heuristic.
Changes:
- Added tests validating
EntityInstance.String()includes key identifiers. - Added tests for
NewEntityWithPropertiesFromInstance,UpdateProperties, andNeedsPropertyLoad. - Added a test validating
EntityWithProperties.String()formatting includes the expected “ENTITY” keyword.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func TestEntityInstanceString_ContainsName(t *testing.T) { | ||
| t.Parallel() | ||
| ei := EntityInstance{ | ||
| ID: uuid.New(), | ||
| Type: minderv1.Entity_ENTITY_REPOSITORIES, | ||
| Name: "myorg/myrepo", | ||
| ProviderID: uuid.New(), |
There was a problem hiding this comment.
PR description lists test cases like TestEntityInstance_FieldsPreserved / TestEntityInstance_ZeroValue / accessor tests, but the added file defines different tests (mostly String()/NeedsPropertyLoad()/UpdateProperties). Please update the PR description (or rename/adjust tests) so the described coverage matches what is actually being added.
| props := properties.NewProperties(map[string]any{"key": "value"}) | ||
| ewp := NewEntityWithPropertiesFromInstance(ei, props) | ||
| if ewp.Entity.Name != ei.Name { | ||
| t.Errorf("entity name = %q, want %q", ewp.Entity.Name, ei.Name) | ||
| } | ||
| if ewp.Properties != props { | ||
| t.Error("properties not stored correctly") | ||
| } |
There was a problem hiding this comment.
TestNewEntityWithPropertiesFromInstance_RoundTrip asserts ewp.Properties == props (pointer identity). This is brittle because a future implementation could legitimately clone/copy properties while preserving behavior. Prefer asserting on observable behavior (e.g., that expected keys/values are present and Len() matches) rather than pointer equality.
| } | ||
| s := ewp.String() | ||
| if !strings.Contains(s, "ENTITY") { | ||
| t.Errorf("EntityWithProperties.String() = %q, want prefix 'ENTITY'", s) |
There was a problem hiding this comment.
The assertion uses strings.Contains(s, "ENTITY") but the failure message says it wants a prefix ENTITY. Either change the check to HasPrefix or adjust the message to match the actual assertion to avoid confusion when debugging failures.
| t.Errorf("EntityWithProperties.String() = %q, want prefix 'ENTITY'", s) | |
| t.Errorf("EntityWithProperties.String() = %q, want to contain 'ENTITY'", s) |
|
It looks like this adds minor coverage for Unfortunately, it seems like |
Summary
Adds a complete unit-test suite for
internal/entities/models, the packagethat defines the core domain structs (
EntityInstance,EntityWithProperties)shared across the rule-engine, reconcilers, and event-bus.
Tests Added
TestEntityInstance_FieldsPreservedEntityInstancesurvive a round-tripTestEntityInstance_ZeroValueTestEntityWithProperties_EntityAccessor.Entityfield returns the embeddedEntityInstanceTestEntityWithProperties_PropertiesAccessor.Propertiesaccessor returns the correct property mapTestEntityWithProperties_NilPropertiesnilproperties map does not panic on accessTestEntityWithProperties_MultiplePropertiesTestEntityInstance_TypeAndProjectPreservedTypeandProjectIDfields survive constructionWhy This Matters
EntityInstanceandEntityWithPropertiesare the most widely-referencedstructs in the Minder rule-engine. Tests here prevent structural regressions
(e.g. a renamed or removed field) from going undetected until runtime.