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

OF-2437: Improve CachingPubsubPersistenceProvider #2664

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

guusdk
Copy link
Member

@guusdk guusdk commented Jan 22, 2025

This new unit test verifies behavior of CachingPubsubPersistenceProvider (which fails, as indicated by some of these test failing).

The tests introduced here only cover one (of three) types of operations that are being scheduled by the implementation:

  • Node creation/changing/removal

These two other types of operation still need to be added:

  • Affiliation creation/update/removal
  • Subscription creation/update/removal

@guusdk
Copy link
Member Author

guusdk commented Jan 22, 2025

@Fishbowler do you think that the test approach is valid? Also, is the behavior (as tested, and documented in the test) consistent and correct?

@guusdk guusdk force-pushed the OF-2437_CachingPubSubPersistenceProvider_fixes branch 6 times, most recently from 565c5f1 to 81c56e8 Compare January 24, 2025 16:11
These new unit tests verify behavior of CachingPubsubPersistenceProvider (which fails, as indicated by some of these test failing).

The tests introduced here cover four types of operations that are being scheduled by the implementation:
- Node creation/changing/removal
- Subscription creation/update/removal
- Affiliation creation/update/removal
- publishing/removal of items
@guusdk guusdk force-pushed the OF-2437_CachingPubSubPersistenceProvider_fixes branch from 81c56e8 to 0a50b8b Compare January 24, 2025 16:32
The changes in this commit address the issues that are exposed in the unit tests that are added in the previous commit.
@guusdk guusdk changed the title OF-2437: Introduce CachingPubsubPersistenceProviderTest OF-2437: Improve CachingPubsubPersistenceProvider Jan 24, 2025
@guusdk
Copy link
Member Author

guusdk commented Jan 24, 2025

The second commit in this PR fixes the issues that are exposed by the unit tests that are added in the first commit.

@guusdk
Copy link
Member Author

guusdk commented Jan 24, 2025

The added unit tests do not verify assert the result of obtaining data from a provider. This is certainly something I'd like to (eventually) add, but this might lead to a refactoring of the interface (the current interface definition requires a caller to pass along an instance of the pubsub service on which the data is to be loaded - that's awkward design, making it hard to write tests for).

@guusdk guusdk marked this pull request as ready for review January 24, 2025 19:09
@guusdk
Copy link
Member Author

guusdk commented Jan 24, 2025

Note that I'm not sure if any of these fixes actually address OF-2437. Given that this issue appears to be specific to the caching provider, I think that there's a good chance. Sadly, we do not know how to reproduce the issue. I think that leaves us with dogfooding, to see if the problem persists after these changes.

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