[test] Improve test coverage and performance in apimachinery_status_conditions #172
Conversation
- Refactored CopyConditions function to avoid unnecessary appends and allocate slice with exact length - Reused CopyConditions in ConditionMarshal instead of using append for performance improvement and null safety
art-tapin
force-pushed
the
issue167/unit-tests-for-apimachinery_status_conditions
branch
from
a08e581
to
379dcdc
Compare
| Message: "Object validation failed", | ||
| }, | ||
| }, | ||
| expectedJSONOutput: `[{"type":"Installed","status":"True","observedGeneration":1,"lastTransitionTime":"` + nowUTC + `","reason":"InstallSuccessful","message":"Object successfully installed"},{"type":"Ready","status":"False","observedGeneration":2,"lastTransitionTime":"` + nowUTC + `","reason":"ComponentsNotReady","message":"Object components are not ready"},{"type":"Validated","status":"Unknown","observedGeneration":3,"lastTransitionTime":"` + nowUTC + `","reason":"ValidationError","message":"Object validation failed"}]`, |
There was a problem hiding this comment.
what? Order does not keep consistent?? First Ready, then Installed and lastly Validated. And for the serialized string: Installed, Ready, Validated. Weird.
HAve you run the test multiple times? if order is not maintained, on multiple runs, some of them could fail
There was a problem hiding this comment.
Thanks for the concern! Conditions are sorted based on their Condition.Type field in the ConditionMarshal function before they're serialized. So, the order is consistent and not random. To answer your question, running the test multiple times should not result in any failures due to the consistent ordering provided by the sorting mechanism. Heer is ConditionMarhsal implementation, pay attention to the sorting part:
func ConditionMarshal(conditions []metav1.Condition) ([]byte, error) {
condCopy := CopyConditions(conditions)
sort.Slice(condCopy, func(a, b int) bool {
return condCopy[a].Type < condCopy[b].Type
})
return json.Marshal(condCopy)
}Let me know if you have any other questions! 👼
There was a problem hiding this comment.
🤦 Sorry for that. Once again I took some assumptions that are wrong.
Looking good
art-tapin
deleted the
issue167/unit-tests-for-apimachinery_status_conditions
branch
This pull request adds unit tests to verify the correctness of the CopyConditions and ConditionMarshal functions in the 'common' package's apimachinery_status_conditions.go file.
It also refactors the CopyConditions function to avoid unnecessary appends and allocate a slice with the exact length.
Finally, it reuses the CopyConditions function in ConditionMarshal instead of using append for performance improvement and null safety. This work partly addresses issue #167, and more work is still in progress.