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

Return updated state in the cache notification event #327

Merged
merged 1 commit into from
Jan 29, 2024

Conversation

feld
Copy link
Contributor

@feld feld commented Jan 26, 2024

when the registered Hook is defined with a timeout value

Fixes #325

@whitfin whitfin added this to the v3.7.0 milestone Jan 26, 2024
@whitfin whitfin added the bug label Jan 26, 2024
@whitfin whitfin self-requested a review January 26, 2024 19:32
@feld
Copy link
Contributor Author

feld commented Jan 26, 2024

I really wanted to write a test to validate this but I struggled to unravel what was going on in test/cachex/services/informant_test.exs. The cache4 or hook4 is the right thing to work with as it is created with a timeout value of 50, but even if I alter the hook binding to have actions: [:all] so it stops erroring when I try to Cachex.put/3 it never seems to generate any messages? Maybe I'm completely misunderstanding these tests

@whitfin
Copy link
Owner

whitfin commented Jan 26, 2024

@feld I wouldn't worry about adding a test here, it's unlikely to ever hit again (and even if it does nobody appears to care).

If you really want to, I'd just make a new test case specifically for this rather than changing the existing ones. The existing ones aren't that simple, but they cover all the basics. As this is something specific, I'd just add a new one - although again, I don't think it's necessary.

LMK either way, happy to merge this.

@feld
Copy link
Contributor Author

feld commented Jan 29, 2024

If you're happy, I'm happy. I'll leave the test case alone.

@whitfin whitfin merged commit a876544 into whitfin:main Jan 29, 2024
15 checks passed
@whitfin whitfin modified the milestones: v3.7.0, v4.0.0 Mar 23, 2024
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.

Possible bug in :cachex_notify event handling?
2 participants