-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Script to run all non-local tests against Confluent Platform or Confluent Cloud #5145
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
base: master
Are you sure you want to change the base?
Conversation
…ause if a minimum replication factor enforced on topic creation
to run shell commands if there's no local broker
🎉 All Contributor License Agreements have been signed. Ready to merge. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Enable running all non-local tests against Confluent Platform/Cloud by refactoring topic‐creation helpers, introducing a new test runner script, and updating CI behavior
- Add
test_can_kafka_cmd
and refactortest_can_create_topics
to fall back to CLI tooling when AdminAPI is unavailable - Change hardcoded replication factor
1
to-1
(broker default) in all test topic‐creation calls - Add
run-tests-confluent-kafka.py
, update dependencies, and adjust Semaphore CI to return code 130 for skipped jobs
Reviewed Changes
Copilot reviewed 43 out of 43 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
tests/testshared.h | Declare new test_can_kafka_cmd(int skip) helper |
tests/test.h | Add prototype for test_can_kafka_cmd |
tests/test.c | Rename/refactor test_can_create_topics into test_can_kafka_cmd , rewrite fallback logic, update AdminClient tests and flags |
tests/run-tests-confluent-kafka.py | New Python script to orchestrate non-local tests against Confluent Kafka |
tests/requirements.txt | Add httpx dependency |
.semaphore/run-all-tests.yml | Change CI skip logic to use return code 130 instead of exit 0 |
tests//*.c, tests//*.cpp | Replace replication factor 1 with -1 in all test_create_topic* calls |
Comments suppressed due to low confidence (1)
.semaphore/run-all-tests.yml:41
- Using
return 130
in a top-level shell command may be invalid; useexit 130
to properly terminate the script with the intended status code.
return 130;
|
||
args = sys.argv[1:] | ||
|
||
if '-l' in args: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The script checks for '-l' but then passes '-L' to run-test-batches.py
. Change the check to if '-L' in args:
(or accept both) so local tests are correctly excluded.
if '-l' in args: | |
if '-l' in args or '-L' in args: |
Copilot uses AI. Check for mistakes.
test_str_id_generate(group_id, sizeof(group_id)); | ||
|
||
test_create_topic_wait_exists(NULL, topic, 1, 1, 5000); | ||
test_create_topic_wait_exists(NULL, topic, 1, -1, 5000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The replication factor argument -1
is a magic number; consider defining a named constant (e.g., DEFAULT_REPLICATION_FACTOR
) or a small wrapper to improve readability and reduce duplication.
test_create_topic_wait_exists(NULL, topic, 1, -1, 5000); | |
test_create_topic_wait_exists(NULL, topic, 1, DEFAULT_REPLICATION_FACTOR, 5000); |
Copilot uses AI. Check for mistakes.
@@ -1,2 +1,3 @@ | |||
trivup/trivup-0.13.0.tar.gz | |||
jsoncomment | |||
httpx |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Pin httpx
to a specific version in requirements.txt
(e.g., httpx==0.24.0
) to avoid unexpected breakage from future releases.
httpx | |
httpx==0.24.0 |
Copilot uses AI. Check for mistakes.
5e25e82
to
e08c8a6
Compare
No description provided.