Skip to content

feat(bdd): add shared stream operations scenario for Go SDK#3063

Open
ex172000 wants to merge 5 commits intoapache:masterfrom
ex172000:qichao/convert-go-bdd-1
Open

feat(bdd): add shared stream operations scenario for Go SDK#3063
ex172000 wants to merge 5 commits intoapache:masterfrom
ex172000:qichao/convert-go-bdd-1

Conversation

@ex172000
Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Related to #1986

Rationale

Adopt the same pattern as other BDD

What changed?

Add a shared Gherkin feature file for stream CRUD operations and corresponding godog step definitions, beginning the migration of Go BDD tests from embedded Ginkgo scenarios to shared .feature files.

Local Execution

  • Passed
  • Pre-commit hooks ran (prek --last-commit passed)

AI Usage

  1. Which tools? Claude Code
  2. Scope of usage? Generate functions under supervision and planning
  3. How did you verify the generated code works correctly? Testing and manual verify
  4. Can you explain every line of the code if asked? Yes

@hubcio
Copy link
Copy Markdown
Contributor

hubcio commented Mar 31, 2026

this is nice. once you add more feature files (tests), can you create issues for implementing them in other SDKs?

@hubcio hubcio changed the title feat(bdd): add shared stream operations scenario for Go SDK (#1986) feat(bdd): add shared stream operations scenario for Go SDK Mar 31, 2026
@chengxilo
Copy link
Copy Markdown
Contributor

chengxilo commented Mar 31, 2026

Finally, another .feature file. 😭

ex172000 and others added 2 commits March 31, 2026 08:18
)

Add a shared Gherkin feature file for stream CRUD operations and
corresponding godog step definitions, beginning the migration of Go
BDD tests from embedded Ginkgo scenarios to shared .feature files.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Move step definitions from stream_operations_test.go to
stream_operations.go (non-test file) and register the suite in
suite_test.go, matching the pattern used by basic_messaging and
leader_redirection. Update imports to use refactored Go SDK paths
(client, client/tcp, contracts).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ex172000 ex172000 force-pushed the qichao/convert-go-bdd-1 branch from 66a0626 to d9d8908 Compare April 2, 2026 20:11
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 2, 2026

Codecov Report

❌ Patch coverage is 0% with 31 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.20%. Comparing base (b19daa9) to head (5db4ccb).

Files with missing lines Patch % Lines
foreign/python/src/client.rs 0.00% 31 Missing ⚠️

❌ Your patch check has failed because the patch coverage (0.00%) is below the target coverage (50.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #3063      +/-   ##
============================================
+ Coverage     70.58%   71.20%   +0.62%     
  Complexity      943      943              
============================================
  Files          1113     1136      +23     
  Lines         94590    98100    +3510     
  Branches      71787    75287    +3500     
============================================
+ Hits          66770    69856    +3086     
- Misses        25348    25538     +190     
- Partials       2472     2706     +234     
Components Coverage Δ
Rust Core 71.51% <ø> (+0.87%) ⬆️
Java SDK 62.30% <ø> (ø)
C# SDK 69.11% <ø> (-0.29%) ⬇️
Python SDK 77.42% <0.00%> (-4.02%) ⬇️
Node SDK 91.39% <ø> (-0.04%) ⬇️
Go SDK 38.97% <ø> (ø)
Files with missing lines Coverage Δ
foreign/python/src/client.rs 82.37% <0.00%> (-9.09%) ⬇️

... and 187 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

ex172000 and others added 2 commits April 2, 2026 15:35
apache#1986)

Extend the shared basic_messaging.feature with stream update and delete
steps, covering full CRUD lifecycle. Add matching step definitions in
all 6 SDKs (Go, Python, Rust, Java, Node, C#). Add update_stream and
delete_stream methods to the Python SDK (PyO3). Remove the separate
stream_operations.feature and its Go-only implementation to avoid
duplication.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Use iggy::prelude::Identifier instead of iggy::identifier::Identifier
and unwrap Option<StreamDetails> from get_stream. Add null assertion
for stream.get() return value in Node step definition.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@ex172000
Copy link
Copy Markdown
Contributor Author

ex172000 commented Apr 3, 2026

this is nice. once you add more feature files (tests), can you create issues for implementing them in other SDKs?

Sure! My thought is to modify on the go so we don't have additional step. In this case, I enhanced the basic messaging to include CRUD for streams so all language can have it now

@ex172000
Copy link
Copy Markdown
Contributor Author

ex172000 commented Apr 3, 2026

Finally, another .feature file. 😭

Thanks for pointing out! I don't like either and modified! Please take another look :D

Comment on lines +245 to +251
func (s basicMessagingSteps) thenStreamDeletedSuccessfully(ctx context.Context) error {
c := getBasicMessagingCtx(ctx)
if c.lastStreamID != nil {
return errors.New("stream ID should be nil after deletion")
}
return nil
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think it's better to call GetStream here to verify whether the stream is actually deleted.

Comment on lines +368 to +369
sc.Step(`I delete the stream`, s.whenDeleteStream)
sc.Step(`the stream should be deleted successfully`, s.thenStreamDeletedSuccessfully)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I recommend ^...$ for consistency. e.g. ^I delete the stream$

Comment on lines +372 to +379
if c.client != nil && c.lastStreamID != nil {
streamIdentifier, _ := iggcon.NewIdentifier(*c.lastStreamID)
_ = c.client.DeleteStream(streamIdentifier)
}
if c.client != nil {
if err := c.client.Close(); err != nil {
scErr = errors.Join(scErr, fmt.Errorf("error closing client: %w", err))
}
Copy link
Copy Markdown
Contributor

@chengxilo chengxilo Apr 3, 2026

Choose a reason for hiding this comment

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

I’m not sure it’s worth cleaning this up. Even if we do want to, this approach isn’t ideal. We should probably provide a script that cleans up all resources instead (e.g., fetch all streams, topics, users, etc., and delete them), which would work across all scenarios and ensure everything is properly cleaned.

For example, if CreateStream(A) actually creates a stream B, but DeleteStream(A) only attempts to delete A, then who is responsible for deleting B?

@chengxilo
Copy link
Copy Markdown
Contributor

Finally, another .feature file. 😭

Thanks for pointing out! I don't like either and modified! Please take another look :D

Actually this is not part of code review LOL. I commented in Go SDK bdd. Would you mind to take a look at them?

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.

4 participants