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 keystone e2e test dispatcher to correctly replicate duplicate reg… #14018

Merged
merged 1 commit into from
Aug 2, 2024

Conversation

ettec
Copy link
Collaborator

@ettec ettec commented Aug 2, 2024

fix keystone e2e test dispatcher to correctly replicate duplicate registration behaviour. The syncer relies on the dispatcher returning a ErrReceiverExists error code to prevent duplicate creation of a capability. The mocked dispatcher used for the e2e test was not replicating this behaviour resulting in duplicate registration of trigger publisher.

@ettec ettec requested a review from a team as a code owner August 2, 2024 13:50
@cedric-cordenier
Copy link
Contributor

@ettec One point of feedback (which shouldn't block this) is that I wonder if we could increase our overall coverage by mocking out the external peer wrapper, rather than just the dispatcher? That would allow us to bring the registrysyncer, launcher and dispatcher in scope and would have eliminated this bug.

krehermann
krehermann previously approved these changes Aug 2, 2024
@ettec ettec added this pull request to the merge queue Aug 2, 2024
@ettec ettec removed this pull request from the merge queue due to a manual request Aug 2, 2024
@ettec
Copy link
Collaborator Author

ettec commented Aug 2, 2024

@ettec One point of feedback (which shouldn't block this) is that I wonder if we could increase our overall coverage by mocking out the external peer wrapper, rather than just the dispatcher? That would allow us to bring the registrysyncer, launcher and dispatcher in scope and would have eliminated this bug.

The registrysyncer and launcher are already used in the test, unless you mean something else by bring them in scope?

@cedric-cordenier
Copy link
Contributor

@ettec Nope, just a brainfart on my end: I meant bringing the dispatcher entirely within scope -- so effectively the only thing out of scope would be the actual networking (+ OCR itself)

@ettec
Copy link
Collaborator Author

ettec commented Aug 2, 2024

@ettec Nope, just a brainfart on my end: I meant bringing the dispatcher entirely within scope -- so effectively the only thing out of scope would be the actual networking (+ OCR itself)

looks like it could work, a brief look at the p2p wrapper usage in the dispatcher would seem to suggest it would be relatively straightforward

@ettec ettec added this pull request to the merge queue Aug 2, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 2, 2024
@ettec ettec added this pull request to the merge queue Aug 2, 2024
Merged via the queue into develop with commit 82accff Aug 2, 2024
117 of 118 checks passed
@ettec ettec deleted the keystone-e2e-test-mock-dispatcher-fix branch August 2, 2024 16:42
Tofel pushed a commit that referenced this pull request Aug 5, 2024
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.

3 participants