Skip to content

fix(bdd): make ./scripts/run-bdd-tests.sh actually support FEATURE flag#3384

Open
chengxilo wants to merge 26 commits into
apache:masterfrom
chengxilo:seperate_bdd_test
Open

fix(bdd): make ./scripts/run-bdd-tests.sh actually support FEATURE flag#3384
chengxilo wants to merge 26 commits into
apache:masterfrom
chengxilo:seperate_bdd_test

Conversation

@chengxilo
Copy link
Copy Markdown
Contributor

@chengxilo chengxilo commented May 31, 2026

Which issue does this PR address?

Relates to #2148

Rationale

In master branch, the ./scripts/run/bdd-tests.sh doesn't support choosing a feature, while there is a FEATURE varaible.
While this script is mainly for BDD test in CI, when we implement the bdd test it is also very helpful to verify if the BDD test works. So I hope this can be improved so when I build new test scenario, it won't take me 10 minutes to build and run everything everytime I wanna test it.

What changed?

In master branch, the ./scripts/run/bdd-tests.sh doesn't support choosing a feature, while there is a FEATURE varaible. I splitted the docker-compose file for each .feature. Now when run basic_messaging scenario we only need to build and run the docker-compose.server.yml which create a single server, while in leader_redirection scenario we use docker-compose.server.yml and docker-compose.cluster.yml together. It should increase the speed to run the run-bdd-tests.sh

the clean is also updated by claude, but I think it makes sense so I kept it. Now if we use ./scripts/run/bdd-tests.sh clean, it should clean up everything that is possible to be booted but failed to be removed. I didn't actually use it tho.

And one extra change which is technically not in the scope, I removed go-race test in bdd, as I realized that it was a mistake to provide -race test in BDD test. It is only applicable for unit test with mock server.

Local Execution

  • Passed
  • Pre-commit hooks ran

AI Usage

  1. Claude Opus 4.6
  2. Code generation, I'm not good at writing shell script so it helps a lot.
  3. The code was manually modified after generation
  4. yes

@github-actions
Copy link
Copy Markdown

Thanks for the PR. It is labeled S-waiting-on-review and queued for review.

Slash commands (own line, regular comment) move it around the queue:

  • /ready - back to S-waiting-on-review after addressing feedback
  • /request-review @user-or-team - request a reviewer

See CONTRIBUTING.md for details.

@github-actions github-actions Bot added the S-waiting-on-review PR is waiting on a reviewer label May 31, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented May 31, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 74.41%. Comparing base (5858410) to head (2b56a33).

Additional details and impacted files
@@             Coverage Diff              @@
##             master    #3384      +/-   ##
============================================
- Coverage     74.46%   74.41%   -0.06%     
  Complexity      943      943              
============================================
  Files          1231     1231              
  Lines        120911   120911              
  Branches      97648    97677      +29     
============================================
- Hits          90042    89973      -69     
- Misses        27918    27952      +34     
- Partials       2951     2986      +35     
Components Coverage Δ
Rust Core 75.57% <ø> (-0.05%) ⬇️
Java SDK 58.44% <ø> (ø)
C# SDK 69.41% <ø> (-0.55%) ⬇️
Python SDK 81.06% <ø> (ø)
Node SDK 91.53% <ø> (+0.01%) ⬆️
Go SDK 40.20% <ø> (ø)
see 30 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.

@chengxilo chengxilo changed the title test: make ./scripts/run-bdd-tests.sh actually support FEATURE flag fix(bdd): make ./scripts/run-bdd-tests.sh actually support FEATURE flag May 31, 2026
Copy link
Copy Markdown
Contributor

@hubcio hubcio left a comment

Choose a reason for hiding this comment

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

one thing to confirm rather than a code issue: this PR drops the go -race BDD task (bdd-go-race, the -race flag and GO_TEST_EXTRA_FLAGS plumbing). no dangling references remain and the CI matrix entry goes away cleanly, so the removal itself is clean. worth a conscious sign-off on losing the data-race signal though - -race instruments the go test binary's own goroutines regardless of whether the server is real or mocked, so the "only applies to unit tests with a mock server" rationale isn't quite right. note coverage-baseline.yml still runs go test -race against tests/tcp_test, so some race coverage survives there.

Comment thread bdd/docker-compose.yml Outdated
Comment thread bdd/docker-compose.coverage.yml Outdated
Comment thread bdd/go/tests/suite_test.go
Comment thread bdd/docker-compose.yml Outdated
Comment thread scripts/run-bdd-tests.sh Outdated
Comment thread scripts/run-bdd-tests.sh Outdated
Comment thread bdd/docker-compose.server.yml Outdated
@github-actions github-actions Bot added S-waiting-on-author PR is waiting on author response and removed S-waiting-on-review PR is waiting on a reviewer labels Jun 1, 2026
@chengxilo
Copy link
Copy Markdown
Contributor Author

chengxilo commented Jun 1, 2026

one thing to confirm rather than a code issue: this PR drops the go -race BDD task (bdd-go-race, the -race flag and GO_TEST_EXTRA_FLAGS plumbing). no dangling references remain and the CI matrix entry goes away cleanly, so the removal itself is clean. worth a conscious sign-off on losing the data-race signal though - -race instruments the go test binary's own goroutines regardless of whether the server is real or mocked, so the "only applies to unit tests with a mock server" rationale isn't quite right. note coverage-baseline.yml still runs go test -race against tests/tcp_test, so some race coverage survives there.

Sorry it was a huge mistake. Fixed.

I removed the -race from the coverage tho, since it only only cause nagative impact and we don't need race detection for coverage report.

@chengxilo chengxilo force-pushed the seperate_bdd_test branch from 7bf8c80 to 1fb4cd8 Compare June 1, 2026 16:15
@chengxilo chengxilo force-pushed the seperate_bdd_test branch from 1fb4cd8 to 2e82506 Compare June 1, 2026 16:31
@chengxilo
Copy link
Copy Markdown
Contributor Author

/ready

@github-actions github-actions Bot added S-waiting-on-review PR is waiting on a reviewer and removed S-waiting-on-author PR is waiting on author response labels Jun 1, 2026
@chengxilo
Copy link
Copy Markdown
Contributor Author

It's kinda funny that the CI system keeps collapse because of the device space.

image

Comment thread .github/config/components.yml
- -c
- >-
go test -v ${GO_TEST_EXTRA_FLAGS}
go test -v
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.

the go coverage command lost ${GO_TEST_EXTRA_FLAGS} that the base docker-compose.yml go-bdd command still has. a compose override replaces the command rather than merging it, so under --coverage go-race and --coverage all the go coverage run no longer gets -race. restore it: go test -v ${GO_TEST_EXTRA_FLAGS:-} -coverpkg=... -coverprofile=/reports/go-bdd-coverage.out ./...

Copy link
Copy Markdown
Contributor Author

@chengxilo chengxilo Jun 2, 2026

Choose a reason for hiding this comment

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

-race and coverage serve different purposes, -race flag won't benefit the coverage flag. So I removed it intentionally. However, I think I can print a log to warn that ./scripts/run-bdd-tests.sh --coverage go-race won't use -race flag.

Comment thread scripts/run-bdd-tests.sh Outdated
Comment thread scripts/run-bdd-tests.sh
)

COMPOSE_FILES=(-f docker-compose.yml)
case "$FEATURE" in
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.

this case matches basic_messaging|leader_redirection|all, which is every value FEATURE can hold (already validated above), so server.yml is added unconditionally - the branch is a no-op. just seed the array: COMPOSE_FILES=(-f docker-compose.yml -f docker-compose.server.yml) and keep the single case below for cluster.yml.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I'm planning to build another scneario for reconnection. I will integrate a server into the iggy-server image to control the network and stop/kill the iggy-server, so the single server service won't be used

Comment thread scripts/run-bdd-tests.sh Outdated
Comment thread bdd/README.md Outdated
Comment thread bdd/docker-compose.cluster.yml Outdated
Comment thread bdd/docker-compose.server.yml
Comment thread bdd/README.md
@github-actions github-actions Bot added S-waiting-on-author PR is waiting on author response and removed S-waiting-on-review PR is waiting on a reviewer labels Jun 2, 2026
@chengxilo chengxilo force-pushed the seperate_bdd_test branch from 713e921 to 0e00c3a Compare June 2, 2026 17:34
@chengxilo chengxilo force-pushed the seperate_bdd_test branch from 112af6c to 2b56a33 Compare June 4, 2026 02:31
@chengxilo chengxilo requested a review from hubcio June 4, 2026 02:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-author PR is waiting on author response

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants