-
Notifications
You must be signed in to change notification settings - Fork 48
By default all Halibut tests will run with the Redis queue #696
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
base: main
Are you sure you want to change the base?
Conversation
var disposableCollection = new DisposableCollection(); | ||
|
||
var redisFacade = new RedisFacade("localhost:" + RedisTestHost.Port(), (Guid.NewGuid()).ToString(), log); | ||
var redisFacade = RedisFacadeBuilder.CreateRedisFacade(port: RedisTestHost.Port()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing this means we can run the tests on a machine that doesn't have redis/docker and instead point it to another machine which does.
{ | ||
IPendingRequestQueue ?pendingRequestQueue = null; | ||
var halibutTimeoutsAndLimits = new HalibutTimeoutsAndLimitsForTestsBuilder().Build(); | ||
halibutTimeoutsAndLimits.PollingQueueWaitTimeout = TimeSpan.FromSeconds(1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The queues respect this timeout passed in this class, so we can just use that instead of making our own queue.
|
||
|
||
// The queues don't all work the same with the Count operator, this account for that. | ||
int baseCount = pendingRequestQueue!.Count; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For redos the count is the number of in flight requests where for in mem queue the count is the number of yet to be collected requests.
It was hard to make the Count thing even work so that any tests could peer into the queue. This is the consequence if that.
#if NET8_0_OR_GREATER | ||
await stream.CopyToAsync(memoryStream, ct); | ||
#else | ||
await stream.CopyToAsync(memoryStream); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code path is unlikely to be ever hit, and will never be hit in prod.
new CancelWhenRequestDequeuedPendingRequestQueueFactory(inner, tokenSourceToCancel, ShouldCancelOnDequeue, OnResponseApplied))) | ||
.WithPendingRequestQueueFactoryBuilder(builder => builder.WithDecorator((_, inner) => | ||
{ | ||
#if NET8_0_OR_GREATER |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Redis queue does not exist for net48
, leaveOpen: true | ||
#endif | ||
)) | ||
using (var bsonDataWriter = new BsonDataWriter(stream) { CloseOutput = false }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We must use BSON to support services that return a guid type.
|
||
[Test] | ||
[LatestClientAndLatestServiceTestCases(testNetworkConditions: false, testListening: false, testWebSocket: false)] | ||
[LatestClientAndLatestServiceTestCases(testNetworkConditions: false, testListening: false, testWebSocket: false, pollingQueuesToTest: PollingQueuesToTest.RedisOnly)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are Redis specific tests so we only want redis.
} | ||
|
||
[Test] | ||
[LatestClientAndLatestServiceTestCases(testNetworkConditions: false, testListening: false, testWebSocket: false, pollingQueuesToTest: PollingQueuesToTest.RedisOnly)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really this test should have existed before hand, but since I added some code around this I added a test to check it.
|
||
namespace Halibut.Tests.Queue.Redis.Utils | ||
{ | ||
public class WithDisposablesDataStreamStorage : IStoreDataStreamsForDistributedQueues |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had to copy the in memory data stream storage to be able to get something to test that disposables are indeed called.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces the Redis queue as the default test configuration for Halibut polling tests, adding a new queue testing dimension to the test framework. The changes enable tests to run with both in-memory and Redis queues when testing the latest client and service versions.
Key changes:
- Added infrastructure to test polling with Redis queues by default alongside in-memory queues
- Fixed two Redis queue bugs: DataStream handling and GUID serialization by switching from JSON to BSON
- Enhanced test framework to support different queue types with proper configuration
Reviewed Changes
Copilot reviewed 31 out of 31 changed files in this pull request and generated 7 comments.
Show a summary per file
File | Description |
---|---|
PendingRequestQueueFactoryExtensionMethods.cs |
New extension methods for modifying and capturing Redis queue creation |
RehydrateWithProgressReporting.cs |
Added support for using DataStream receiver vs writer based on useReceiver flag |
MessageSerialiserAndDataStreamStorage.cs |
Updated to pass useReceiver parameter to distinguish request vs response handling |
IRehydrateDataStream.cs |
Added DataStreamRehydrationDataDataStreamReceiver class for receiver-based rehydration |
IStoreDataStreamsForDistributedQueues.cs |
Added useReceiver parameter to StoreDataStreams method |
QueueMessageSerializer.cs |
Switched from JSON to BSON serialization for Redis queue compatibility |
Test attribute files | Enhanced test case generation to support different polling queue types |
Test builder files | Updated to support queue type selection and configuration |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
source/Halibut/Queue/QueuedDataStreams/IStoreDataStreamsForDistributedQueues.cs
Outdated
Show resolved
Hide resolved
/// <param name="useReciever">When set 'true' the data must be read from the Receiver of the DataStream. When 'false' use the WriteData() method. | ||
/// This will be true for responses and false for Requests.</param> | ||
/// <param name="cancellationToken"></param> | ||
/// <returns>A string, DataStreamMetadata, containing a small amount of data that will be stored in redis, this will be | ||
/// given to RehydrateDataStreams</returns> | ||
public Task<byte[]> StoreDataStreams(IReadOnlyList<DataStream> dataStreams, CancellationToken cancellationToken); | ||
public Task<byte[]> StoreDataStreams(IReadOnlyList<DataStream> dataStreams, bool useReciever, CancellationToken cancellationToken); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected spelling of 'useReciever' to 'useReceiver'.
Copilot uses AI. Check for mistakes.
source/Halibut.Tests/Queue/Redis/Utils/InMemoryStoreDataStreamsForDistributedQueues.cs
Outdated
Show resolved
Hide resolved
source/Halibut.Tests/Queue/Redis/Utils/InMemoryStoreDataStreamsForDistributedQueues.cs
Outdated
Show resolved
Hide resolved
source/Halibut.Tests/Queue/Redis/Utils/WithDisposablesDataStreamStorage.cs
Outdated
Show resolved
Hide resolved
source/Halibut.Tests/Queue/Redis/Utils/WithDisposablesDataStreamStorage.cs
Outdated
Show resolved
Hide resolved
source/Halibut/Queue/Redis/MessageStorage/MessageSerialiserAndDataStreamStorage.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 31 out of 31 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
source/Halibut/Queue/Redis/MessageStorage/RehydrateWithProgressReporting.cs
Outdated
Show resolved
Hide resolved
source/Halibut.Tests/PollingTentacleDequeuesRequestsInOrderFixture.cs
Outdated
Show resolved
Hide resolved
@@ -0,0 +1 @@ | |||
.git/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lol, I suppose this is logical. I'm surprised this was needed now, where it wasn't needed before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The profanity filter got updated by someone on the internet to include the word dumb. But git makes a file with the word dumb in it when you check out the directory.
It is somewhat of a cock up by that author, to not verify that the tool used by maaaaaany devs doesn't use the word dumb.
…ture.cs Co-authored-by: Copilot <[email protected]>
…sReporting.cs Co-authored-by: Copilot <[email protected]>
Background
Most Halibut tests have attributed like:
LatestClientAndLatestServiceTestCases
which result in that test running under a range of combinations e.g. listening, polling, polling web sockets, good networks, networks with latency etc.With chance we add another combination which is to test polling with the Redis Pending Request Queue.
Doing so means we get to exercise the queue under a range of different conditions doing so has revealed two flaws in the queue:
In terms of when we test with redis:
How to review this PR
Quality ✔️
Pre-requisites