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: fix KeyValueDaoTest #21

Merged
merged 1 commit into from
Oct 17, 2024
Merged

fix: fix KeyValueDaoTest #21

merged 1 commit into from
Oct 17, 2024

Conversation

mrnagydavid
Copy link
Contributor

@mrnagydavid mrnagydavid commented Oct 17, 2024

Not to be merged (probably)

Problem statement

this was the original shape of the testEntries
Screenshot 2024-10-17 at 17 24 49

this is the current:
Screenshot 2024-10-17 at 17 25 14

with this change the redis-lib manual tests (running this keyValueDaoTest()) fail:

Screenshot 2024-10-17 at 17 22 52 Screenshot 2024-10-17 at 17 23 12

With the reversion shown in this PR, every test passes.

Since the Dao is defined in this lib and not in redis-lib, I don't think the issue should be fixed in redis-lib.

The any type is a smell, as it should be Buffer - but then it raises other type errors.

@mrnagydavid
Copy link
Contributor Author

Another fix I found is to use the commonKeyValueDaoDeflatedJsonTransformer in the Dao.
But that also feels like the wrong path, and the Dao test for streamValues still fails - and I didn't figure out why yet.

@kirillgroshkov kirillgroshkov merged commit 8582a1c into master Oct 17, 2024
1 check passed
@kirillgroshkov kirillgroshkov deleted the fix/fix-tests branch October 17, 2024 18:31
@kirillgroshkov
Copy link
Member

Yes, you're right that I forgot to change back to Buffer, and it probably causes issues. I'll look further now and possibly change more things so the test would "look right".

Copy link

🎉 This PR is included in version 9.23.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants