Conversation
|
Really appreciate the great work @thangnguyen-69 ! I've gone through the code and detail and added a number of comments. I think there are a bunch of small things to do but overall the code looks pretty close! 2 other asks: |
| var lock2 = new EtcdLeaseDistributedLock(client, "etcd"); | ||
| await using var handle2 = await lock2.TryAcquireAsync(); | ||
| Assert.That(handle2, Is.Not.Null, "Failed to acquire lock"); | ||
| } |
There was a problem hiding this comment.
I don't think these 3 test cases add anything that isn't covered by the combinatorial cases. Instead, use this class to test for any Etcd-specific behavior.
Notably, we should test all constructor argument validations here, including for the options classes!
src/DistributedLock.Tests/Tests/Etcd/EtcdSynchronizationProviderTest.cs
Outdated
Show resolved
Hide resolved
src/DistributedLock.Tests/Tests/Etcd/EtcdSynchronizationProviderTest.cs
Outdated
Show resolved
Hide resolved
|
madelson
left a comment
There was a problem hiding this comment.
Thanks for the contribution @thangnguyen-69 ! This is off to a great start!
f37dbd6 to
854cb97
Compare
|
@madelson received your comments and i will work on it and notify back again for a 2nd round. |
For #119