🌱 (catalogd) add unit tests for indexing algo for query endpoint #1702
Conversation
✅ Deploy Preview for olmv1 ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| @@ -50,20 +50,6 @@ func (s *section) UnmarshalJSON(b []byte) error { | |||
| return nil | |||
| } | |||
|
|
|||
| func (i index) Size() int64 { | |||
There was a problem hiding this comment.
why are we removing this one?
There was a problem hiding this comment.
Good question. I realized it's not being used anywhere, so at least in the context of existing code, it's dead code.
There was a problem hiding this comment.
Yep, I was using that in my prototype to estimate how big the index would be in memory. No need for it anymore.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1702 +/- ##
==========================================
+ Coverage 67.47% 67.66% +0.18%
==========================================
Files 59 59
Lines 5003 4991 -12
==========================================
+ Hits 3376 3377 +1
+ Misses 1380 1367 -13
Partials 247 247
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
anik120
force-pushed
the
query-endpoint-index-unit-tests
branch
from
24bc1a1 to
b280e81
Compare
camilamacedo86
changed the title
Closes operator-framework#1697 Signed-off-by: Anik Bhattacharjee <[email protected]>
anik120
force-pushed
the
query-endpoint-index-unit-tests
branch
from
b280e81 to
8bf3528
Compare
|
HI @anik120 Nit: I hope that you do not mind I update the emoji with the title to 🌱 because that means no changes to end users Just sort out the lint then it seems good to fly 🚀 |
|
@camilamacedo86 thanks for the lgtm! lint is passing now too. |
| } | ||
|
|
||
| // createBlob is a helper function that creates a JSON blob with a trailing newline | ||
| func createBlob(t *testing.T, data map[string]interface{}) []byte { |
There was a problem hiding this comment.
Could also use the declcfg.WriteJSON method in operator-registry which utilizes the underlying JSON encoder which turns each blob into a JSONLines-compliant format by appending a newline suffix here.
This is how catalogd ends up coincidentally JSONLines-compliant.
There was a problem hiding this comment.
I studied the WriteJSON function and looks like it requires a declCfg.DeclConfig as an argument. Overkill imho if we create a declCfg.DeclConfig from test data just to use WriteJSON.
There was a problem hiding this comment.
This is how catalogd ends up coincidentally JSONLines-compliant.
On a side note this sounds fragile because it looks like they only add the /n to make the output look nicer and could easily pull this out in a future release in which case WriteJSON will stop being JSONLines compliant.
Not sure how many places this function is actually being used, but I guess at this point this is a "we'll cross the bridge if we ever get there" case :)
There was a problem hiding this comment.
Yeah, I initially included the note just for the context because this is totally an accident and could just as easily go away. So we used to have tests to ensure we would recognize any change from that accident.
We need to restore those tests, since presumably any behavior change will come from a dependency bump, so we want the CI to fail early/loud if the behavior changes.
There was a problem hiding this comment.
These aren't related to this PR, so I created #1709 for them.
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| reader := idx.Get(fullData, tt.schema, tt.packageName, tt.blobName) |
There was a problem hiding this comment.
blobName and packageName are not consistently provided in the tests. Are they needed?
Looks like blobName is actually bundleName. Rename?
There was a problem hiding this comment.
getSection() code handles these cases:
func (i *index) getSectionSet(schema, packageName, name string) sets.Set[section] {
.
.
// Filter by package name if specified
if packageName != "" {
.
.
}
// Filter by name if specified
if name != "" {
.
.
}
return sectionSet
}
There was a problem hiding this comment.
/lgtm
@grokspawn, it seems fine to me.
@anik120 shows that answers your concerns.
But @anik120 I think it would be nice just ensure that @grokspawn is also OK within
Closes #1697
Description
Reviewer Checklist