[ENH]: Add property test log service #1969
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @nicolasgere and the rest of your teammates on |
|
Please tag your PR title with one of: [ENH | BUG | DOC | TST | BLD | PERF | TYP | CLN | CHORE]. See https://docs.trychroma.com/contributing#contributing-code-and-ideas |
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
nicolasgere
force-pushed
the
04-01-add_property_test
branch
from
e6e5475
to
b70396e
Compare
nicolasgere
changed the title
| CollectionId: c.String(), | ||
| StartFromOffset: startOffset, | ||
| BatchSize: batchSize, | ||
| EndTimestamp: time.Now().Unix(), |
There was a problem hiding this comment.
Can we also generate the EndTimestamp?
There was a problem hiding this comment.
yes, once I have it deploy to staging, i would like to pass even more time on making the property test even better, like parallelism, more logs, use timestamp etc
| suite.model.CollectionData[c] = append(suite.model.CollectionData[c], records...) | ||
| }, | ||
| "getAllCollectionsToCompact": func(t *rapid.T) { | ||
| result, err := suite.logServer.GetAllCollectionInfoToCompact(ctx, &logservicepb.GetAllCollectionInfoToCompactRequest{}) |
There was a problem hiding this comment.
Can we add more sanity check for getAllCollectionsToCompact? One check could be the checking the number of collections to compact matches the number of collections in the model.
| if startOffset == 0 { | ||
| startOffset = 1 | ||
| } | ||
| batchSize := rapid.Int32Range(1, 20).Draw(t, "batch_size") |
There was a problem hiding this comment.
A couple of questions:
- What is the behavior if batchSize <= 1? Especially batchSize <= 0?
- What is the reasoning behind using (1, 20) as the batchSize range?
There was a problem hiding this comment.
it was just way to have random number, I don't know what it would be in production.
| } | ||
| } | ||
|
|
||
| // Verify that the first and last record offset is correct |
There was a problem hiding this comment.
Can we verify the offset in the middle of the batch as well?
There was a problem hiding this comment.
if the number count and the first and last offset is correct, I expect the offset to be correct for all of them.
nicolasgere
force-pushed
the
04-01-add_property_test
branch
from
6983c2a
to
b012c37
Compare
*Summarize the changes made by this PR.* - Improvements & Bug fixes - Add property testing for log service
## Description of changes *Summarize the changes made by this PR.* - Improvements & Bug fixes - When revamping the prop tests in #1969 I noticed that the purge code was off by one - this remedies that. - New functionality - None ## Test plan *How are these changes tested?* The tests TODO is updated with the correct behavior - [x] Tests pass locally with `pytest` for python, `yarn test` for js, `cargo test` for rust ## Documentation Changes None
Description of changes
Summarize the changes made by this PR.