Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: always use slice() for applying offset and limit in loki storage instance, fixing the fuzzing test run #5757

Merged
merged 7 commits into from
Mar 12, 2024

Conversation

Connum
Copy link
Contributor

@Connum Connum commented Mar 10, 2024

This PR contains:

A BUGFIX

Describe the problem you have without this PR

The fuzzy testing failed for the LokiJS storage, because sometimes there were less documents returned than expected. After quite some investigation, I found limit() and offset() to be the culprits, so instead of using those, slice() is now always used for limit and offset values, just as it already was when mustRunMatcher was true.

Todos

  • Changelog

@Connum Connum force-pushed the fix/lokijs-fuzzing branch from 0daa350 to 8e8a0e5 Compare March 10, 2024 18:52
@pubkey
Copy link
Owner

pubkey commented Mar 10, 2024

Hi. The changes look good. Can you add one test to the query-correctness tests which contains the broken fuzzed query+documents combination?

@Connum
Copy link
Contributor Author

Connum commented Mar 11, 2024

Sure! I'm currently trying to track down another loki related bug, then I'll add the test for this one.

@Connum Connum force-pushed the fix/lokijs-fuzzing branch from 252521f to cca6c1f Compare March 12, 2024 08:13
@Connum
Copy link
Contributor Author

Connum commented Mar 12, 2024

Test has been added and verified that it fails before the fix and passes after.

@Connum
Copy link
Contributor Author

Connum commented Mar 12, 2024

I guess the failing example-graphq jobl must have been a hiccup during npm install, it shouldn't have anything to do with this PR, but a re-run might bring clarity.

@pubkey
Copy link
Owner

pubkey commented Mar 12, 2024

Yes I will just retry the CI until it works.

@pubkey pubkey merged commit f8f9ae2 into pubkey:master Mar 12, 2024
21 checks passed
pubkey added a commit that referenced this pull request Mar 12, 2024
@pubkey
Copy link
Owner

pubkey commented Mar 12, 2024

Merged. Thank you for that great contribution.

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