test(csharp): add leader_redirection scenario to BDD tests#2948
test(csharp): add leader_redirection scenario to BDD tests#2948yeyomontana wants to merge 19 commits intoapache:masterfrom
Conversation
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.36% 71.80% +1.44%
Complexity 925 925
============================================
Files 1050 1113 +63
Lines 86942 92584 +5642
Branches 64492 70145 +5653
============================================
+ Hits 61178 66484 +5306
- Misses 23256 23535 +279
- Partials 2508 2565 +57
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
@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. |
|
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
Show resolved
Hide resolved
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.
foreign/csharp/Iggy_SDK.Tests.BDD/StepDefinitions/LeaderRedirectionSteps.cs
Outdated
Show resolved
Hide resolved
foreign/csharp/Iggy_SDK.Tests.BDD/StepDefinitions/LeaderRedirectionSteps.cs
Outdated
Show resolved
Hide resolved
foreign/csharp/Iggy_SDK.Tests.BDD/StepDefinitions/LeaderRedirectionSteps.cs
Outdated
Show resolved
Hide resolved
foreign/csharp/Iggy_SDK.Tests.BDD/StepDefinitions/LeaderRedirectionSteps.cs
Outdated
Show resolved
Hide resolved
foreign/csharp/Iggy_SDK.Tests.BDD/StepDefinitions/LeaderRedirectionSteps.cs
Outdated
Show resolved
Hide resolved
|
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. |
|
Applied the fix and pushed an update. Verified locally:
Thank you for the feedback. |
|
All requested changes have been addressed:
CI is now passing locally and in GitHub. Would appreciate a re-review when you have a moment. |
lukaszzborek
left a comment
There was a problem hiding this comment.
@yeyomontana I added 2 small cleanup comments. Also please update branch from master. After that i think we can merge it
foreign/csharp/Iggy_SDK.Tests.BDD/StepDefinitions/LeaderRedirectionSteps.cs
Outdated
Show resolved
Hide resolved
1805447 to
2bbdb03
Compare
|
@lukaszzborek removed the two unused pieces you pointed out. The branch is already up to date with master and ready for merge. |
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