Skip to content

Improve embedding test coverage#115

Merged
Vladyslav-Kuksiuk merged 4 commits into
masterfrom
improve-embedding-test-coverage
Jul 13, 2026
Merged

Improve embedding test coverage#115
Vladyslav-Kuksiuk merged 4 commits into
masterfrom
improve-embedding-test-coverage

Conversation

@Vladyslav-Kuksiuk

Copy link
Copy Markdown
Collaborator

This PR improves the embedding package test coverage.

@Vladyslav-Kuksiuk Vladyslav-Kuksiuk self-assigned this Jul 12, 2026
@codecov

codecov Bot commented Jul 12, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.61%. Comparing base (62132b0) to head (af65930).

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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Vladyslav-Kuksiuk Vladyslav-Kuksiuk marked this pull request as ready for review July 13, 2026 09:01
Comment thread embedding/processor.go
// 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)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

It is the main reason of having this newDefaultResolver to not raise error, we shouldn't panic in this case.

@Oleg-Melnik Oleg-Melnik left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@Vladyslav-Kuksiuk LGTM with a minor comment.

@Vladyslav-Kuksiuk Vladyslav-Kuksiuk merged commit 9586f2f into master Jul 13, 2026
5 checks passed
@Vladyslav-Kuksiuk Vladyslav-Kuksiuk deleted the improve-embedding-test-coverage branch July 13, 2026 11:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants