Improve embedding test coverage#115
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #115 +/- ##
==========================================
+ Coverage 82.37% 88.61% +6.23%
==========================================
Files 43 43
Lines 2951 2950 -1
==========================================
+ Hits 2431 2614 +183
+ Misses 390 250 -140
+ Partials 130 86 -44 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| // newDefaultResolver creates a resolver with the fixed positive default cache limit. | ||
| // The constructor cannot fail because DefaultResolverCacheLimit satisfies its precondition. | ||
| func newDefaultResolver() *fragmentation.Resolver { | ||
| resolver, _ := fragmentation.NewResolver(fragmentation.DefaultResolverCacheLimit) |
There was a problem hiding this comment.
Low-severity / robustness: swallowing the error and returning on the blank identifier means that if the invariant ever breaks — e.g. a future change lowers DefaultResolverCacheLimit below 1, or NewResolver grows a new failure mode — this returns a nil *Resolver, and the callers (NewProcessor, processRequiredDocs) will dereference it and panic far from the real cause. The comment correctly documents why it cannot fail today, but consider failing fast at the source instead:
resolver, err := fragmentation.NewResolver(fragmentation.DefaultResolverCacheLimit)
if err != nil {
panic(fmt.Sprintf("default resolver cache limit %d is invalid: %v", fragmentation.DefaultResolverCacheLimit, err))
}That turns a silent nil-deref-later into an immediate, self-explaining failure the moment the precondition is violated. Not blocking — the current code is correct as long as the constant stays ≥ 1.
There was a problem hiding this comment.
It is the main reason of having this newDefaultResolver to not raise error, we shouldn't panic in this case.
Oleg-Melnik
left a comment
There was a problem hiding this comment.
@Vladyslav-Kuksiuk LGTM with a minor comment.
This PR improves the
embeddingpackage test coverage.