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: write correct value for KV.Release #237

Merged
merged 9 commits into from
Sep 6, 2023
Merged

fix: write correct value for KV.Release #237

merged 9 commits into from
Sep 6, 2023

Conversation

winstxnhdw
Copy link
Contributor

Copy link
Contributor

@marcin-krystianc marcin-krystianc left a comment

Choose a reason for hiding this comment

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

In the KV_AcquireRelease test, I would also:

  • assert that the value in the acquired KV is "test"
  • set the current value to "test2"
  • assert that the value in the released KV is "test2"

Consul.Test/KVTest.cs Outdated Show resolved Hide resolved
Consul/KV.cs Outdated Show resolved Hide resolved
winstxnhdw and others added 4 commits September 5, 2023 04:15
Co-authored-by: Marcin Krystianc <marcin.krystianc@gmail.com>
Co-authored-by: Marcin Krystianc <marcin.krystianc@gmail.com>
[Theory]
[InlineData("test")]
[InlineData("test2")]
public async Task KV_AcquireRelease(string stringValue)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that the test parameters here don't add any value. My intention in the previous comment was that we use different text values for Acquire and for Release, so we can check what value is being written by Release.

Copy link
Contributor Author

@winstxnhdw winstxnhdw Sep 5, 2023

Choose a reason for hiding this comment

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

Not exactly sure if I understand what the difference would be.

Copy link
Contributor

@marcin-krystianc marcin-krystianc Sep 5, 2023

Choose a reason for hiding this comment

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

Not exactly sure if I understand what the difference would be.

I want to assert that Release operation releases the KV but also writes a new value.

Session = id
};

await _client.KV.Acquire(newPair);
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to call the Acquire again.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without acquiring again, the test will fail..

Copy link
Contributor

Choose a reason for hiding this comment

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

Strange, I'm sure I tested it locally yesterday and it worked, but now it seems it doesn't work anymore, investigating...

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I found it. You have the release operation twice, but it should be called only once like this:

 [Fact]
        public async Task KV_AcquireRelease()
        {
            var sessionRequest = await _client.Session.CreateNoChecks(new SessionEntry());
            var id = sessionRequest.Response;

            Assert.False(string.IsNullOrEmpty(sessionRequest.Response));

            var key = GenerateTestKeyName();
            var value = "test";

            var pair = new KVPair(key)
            {
                Value = Encoding.UTF8.GetBytes(value),
                Session = id,
            };

            var acquireRequest = await _client.KV.Acquire(pair);
            Assert.True(acquireRequest.Response);

            var getRequest = await _client.KV.Get(key);

            Assert.NotNull(getRequest.Response);
            Assert.Equal(id, getRequest.Response.Session);
            Assert.Equal(getRequest.Response.LockIndex, (ulong)1);
            Assert.True(getRequest.LastIndex > 0);

            pair.Value = Encoding.UTF8.GetBytes("test2");
            var releaseRequest = await _client.KV.Release(pair);
            Assert.True(releaseRequest.Response);

            getRequest = await _client.KV.Get(key);
            Assert.NotNull(getRequest.Response);
            Assert.Equal("test2", Encoding.UTF8.GetString(getRequest.Response.Value));
            Assert.Null(getRequest.Response.Session);
            Assert.Equal(getRequest.Response.LockIndex, (ulong)1);
            Assert.True(getRequest.LastIndex > 0);

            var sessionDestroyRequest = await _client.Session.Destroy(id);
            Assert.True(sessionDestroyRequest.Response);

            var deleteRequest = await _client.KV.Delete(key);
            Assert.True(deleteRequest.Response);
        }

Consul.Test/KVTest.cs Outdated Show resolved Hide resolved
Consul.Test/KVTest.cs Outdated Show resolved Hide resolved
winstxnhdw and others added 2 commits September 5, 2023 16:50
Co-authored-by: Marcin Krystianc <marcin.krystianc@gmail.com>
@marcin-krystianc marcin-krystianc merged commit bf65afa into G-Research:master Sep 6, 2023
147 checks passed
@winstxnhdw winstxnhdw deleted the kv-release branch September 23, 2023 16:17
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.

KV.Release is writing a wrong value
2 participants