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

SOL-112355: Integration tests to cover the processing of requests and responses #44

Merged
merged 5 commits into from
Mar 13, 2024

Conversation

oodigie
Copy link

@oodigie oodigie commented Mar 12, 2024

Changes:

  • SOL-112354: Start and termination of a RequestReplyMessageReceiver integration tests
  • feat: updated the tests to fix potential race conditions with the use of time.Millisecond
  • feat: added an assertion to check the error returned from replier.Reply in one of the tests
  • feat: fixed the failing tests to check that error returns from replier.Reply after service termination
  • SOL-112355: Integration tests to cover the requests and responses processing using the RequestReplyMessagePublisher and RequestReplyMessageReceiver
  • SOL-112355: updated the integration tests to provide a more consistent test outcome

oodigie added 2 commits March 11, 2024 21:58
…cessing using the RequestReplyMessagePublisher and RequestReplyMessageReceiver
Copy link

@cjwmorgan-sol cjwmorgan-sol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a failing test due to a receiver configuration issue.
@oodigie please address it.

test/request_reply_message_receiver_test.go Outdated Show resolved Hide resolved
@cjwmorgan-sol
Copy link

With PR #43 merged this PR might need to sync with the base feature branch SOL-62455 now.

@oodigie
Copy link
Author

oodigie commented Mar 12, 2024

I had hoped it wouldn't but I had to make an update from the #43 for this PR so I guess it caused the conflict. I will resolve the conflict now. And yes this PR (#44) has all the changes from #43

@oodigie oodigie requested a review from cjwmorgan-sol March 12, 2024 19:11
Copy link

@cjwmorgan-sol cjwmorgan-sol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good and git actions passed.
But jenkins show request reply failure so there must some issue in here.
@oodigie please take look.
Since these test pass on relatively clean nodes I suspect this is timing related on busy nodes.

@oodigie
Copy link
Author

oodigie commented Mar 13, 2024

@cjwmorgan-sol,
I am currently publishing 1000 messages for these tests and I also configured the receiver back pressure to have a buffer of 1000 messages (to accommodate these messages). The two test failures are due to timeout waiting for the reply for the published messages - "Timed out waiting to receive reply message". Apparently I do not see this failures on my local machine so it may be due to stress of the test nodes and/or available compute resource.

Seeing as the average time to typically receive a reply per message is about < 1 second, probably < 100 milliseconds (changes per version of the broker), I will either have to increase the timeout to allow receiving all the replies or reduce the amount of messages being published. I will reduce the total published message to 500.

@oodigie oodigie requested a review from cjwmorgan-sol March 13, 2024 18:19
Copy link

@cjwmorgan-sol cjwmorgan-sol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All check passing in git actions, and the only jenkins failure were independent of these changes (all known intermittent).
This looks good to me.
And is ready for merge.

@cjwmorgan-sol cjwmorgan-sol merged commit 2f33dab into SOL-62455 Mar 13, 2024
4 checks passed
@cjwmorgan-sol cjwmorgan-sol deleted the SOL-111699--SOL-112355 branch March 13, 2024 19:36
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