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

Add asynchronous API tests #1010

Merged
merged 2 commits into from
Nov 18, 2021
Merged

Add asynchronous API tests #1010

merged 2 commits into from
Nov 18, 2021

Conversation

bjosv
Copy link
Contributor

@bjosv bjosv commented Oct 25, 2021

Add asynchronous tests for pubsub (RESP2)

This testcase verifies the asynchronous handling of the pubsub flow.
The testcase subscribes to a channel while a second client is used to publish a message on the channel.
After receiving the published message the testcase will unsubscribe and disconnect.

This testcase can be modified to reproduce an issue with having an incoming published message while unsubscribing to the channel.

@bjosv
Copy link
Contributor Author

bjosv commented Oct 25, 2021

@michael-grunder Would async api testing be something that I should continue to pursue? Any thoughts?

@bjosv bjosv changed the title Add asynchronous tests Add asynchronous API tests Oct 25, 2021
@michael-grunder
Copy link
Collaborator

@bjosv Yes, it would be great to have async tests!

Asynchronous testcases that requires the event library `libevent`
can be built and enabled by using the added build flags:
- ENABLE_ASYNC_TESTS when using CMake
- TEST_ASYNC when using Make

The async tests are disabled by default to avoid adding new requirements,
but the testcases are built and run in CI.
@bjosv bjosv force-pushed the async-tests branch 2 times, most recently from 0b1af42 to 869eea1 Compare October 26, 2021 11:53
@bjosv bjosv marked this pull request as ready for review October 26, 2021 12:03
@bjosv
Copy link
Contributor Author

bjosv commented Oct 26, 2021

My plan is to add the corresponding test for RESP3 in a separate PR after this is delivered.

Copy link
Contributor

@zuiderkwast zuiderkwast left a comment

Choose a reason for hiding this comment

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

LGTM

I suppose we could test more async things than just pubsub (e.g. blocking commands like brpop?) but this is a good start.

There are some problems with pubsub callback for resp3 in #967 and #968 so this is a very good start to be able to solve those.

The testcase will subscribe to a channel, and via a second client
a message is published to the channel. After receiving the message
the testcase will unsubscribe and disconnect.
@bjosv
Copy link
Contributor Author

bjosv commented Oct 27, 2021

Remove the use of libevent timer-events for publishing messages, this to mitigate any (yet unseen) timing issues.

@bjosv
Copy link
Contributor Author

bjosv commented Nov 8, 2021

@michael-grunder Is this inline with your thoughts regarding async testing?

@michael-grunder
Copy link
Collaborator

@bjosv Yes, they're great.

Do you have any issues with my minor tweaks in 7ad38dc? Without the changes I run into some compilation issues building on a system with both libev and libevent.

I didn't look in detail but it seems related to this issue

@bjosv
Copy link
Contributor Author

bjosv commented Nov 18, 2021

@michael-grunder Your change looks fine to me.

I have both libev and libevent installed, but it seems that my installed libev-dev apt-package on Ubuntu 20.04 don't contain the file event.h. When I now check libevents file event.h it states that <event2/event.h> should be used, so your change is the right way to do it for many reasons!

@michael-grunder michael-grunder merged commit 4021726 into redis:master Nov 18, 2021
@michael-grunder
Copy link
Collaborator

Merged, thanks!

@zuiderkwast zuiderkwast deleted the async-tests branch November 18, 2021 18:07
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