test(csharp): add leader_redirection scenario to BDD tests#2948
test(csharp): add leader_redirection scenario to BDD tests#2948yeyomontana wants to merge 9 commits intoapache:masterfrom
Conversation
yeyomontana
changed the title
yeyomontana
force-pushed
the
feat/csharp-bdd-leader-redirection
branch
from
812b07f to
b0f1f7e
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #2948 +/- ##
============================================
+ Coverage 70.37% 70.39% +0.02%
Complexity 925 925
============================================
Files 1029 1029
Lines 85281 85279 -2
Branches 62655 62664 +9
============================================
+ Hits 60015 60033 +18
+ Misses 22762 22731 -31
- Partials 2504 2515 +11
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
yeyomontana
changed the title
|
@yeyomontana this PR title is invalid, the CI won't pass. Can you rename the PR title from one of these options:
Please look at other PRs for reference. |
yeyomontana
changed the title
lukaszzborek
changed the title
|
Hi, I see unrelated changes from rust file. Please remove it. |
|
Thanks for the feedback, I removed the unrelated change so the PR now only contains the C# BDD files. I’ve also answered the LLM usage question in the PR description. |
core/integration/tests/data_integrity/verify_no_plaintext_credentials_on_disk.rs
Outdated
Show resolved
Hide resolved
lukaszzborek
left a comment
lukaszzborek
left a comment
There was a problem hiding this comment.
@yeyomontana I added some comments. I propose creating client test metadata where you can store information about the initial address or redirection.
You can also check what it looks like in other languages. Rust and Go have implemented that.
| [Given(@"I have cluster configuration enabled with (\d+) nodes")] | ||
| public void GivenIHaveClusterConfigurationEnabledWithNodes(int nodeCount) | ||
| { | ||
| nodeCount.ShouldBe(2); |
There was a problem hiding this comment.
it's information how many nodes we have. We should store it not check if scenario have 2 nodes
| [Given(@"node (\d+) is configured on port (\d+)")] | ||
| public void GivenNodeIsConfiguredOnPort(int nodeId, int port) | ||
| { | ||
| _ = nodeId; |
There was a problem hiding this comment.
we should store node information in context. nodeId can be key of dictionary or something
| }; | ||
| } | ||
|
|
||
| private static async Task WaitForServerPortAsync(string address) |
There was a problem hiding this comment.
We don't need that. Docker Compose will wait for the server to start.
| [Then(@"the client should not perform any redirection")] | ||
| public void ThenTheClientShouldNotPerformAnyRedirection() | ||
| { | ||
| GetClient("main").GetCurrentAddress().ShouldBe(_context.InitialAddress); |
There was a problem hiding this comment.
I think information about redirection should be in some test client metadata, where you can store initial address for specific client
| [Then(@"client ([A-Z]) should stay connected to port (\d+)")] | ||
| public async Task ThenClientShouldStayConnectedToPort(string clientName, int port) | ||
| { | ||
| await AssertClientAddress(clientName, port); | ||
| } | ||
|
|
||
| [Then(@"client ([A-Z]) should redirect to port (\d+)")] | ||
| public async Task ThenClientShouldRedirectToPort(string clientName, int port) | ||
| { | ||
| await AssertClientAddress(clientName, port); | ||
| } |
There was a problem hiding this comment.
Both steps have the same code. It can be merge into single step with client ([A-Z]) should (?:redirect to|stay connected to) port (\d+)
|
Thanks for the detailed review — I’ve applied all the suggested changes: Let me know if anything still needs adjustment 👍 |
|
@yeyomontana please make sure the code compiles and the tests pass. You should also do this locally. |
@yeyomontana It'll be awesome if you could use the PR template for future PRs to maintain consistency. It'll be there when you open a new PR. |
4 participants
Summary
Add support for the
leader_redirectionBDD scenario in the C# SDK test suite.Changes
LeaderRedirectionSteps.cswith step definitions for the scenarioLLM usage: LLM tools were used to help inspect the repository and assist with debugging CI/compiler issues. The final changes were reviewed and verified manually before submission.
Notes
This adds the missing C# BDD scenario support to align coverage with the existing shared scenario set.
Closes #2628