[SPARK-56403] Refactor kafka test so it's skipped when dependency is not available#55266
[SPARK-56403] Refactor kafka test so it's skipped when dependency is not available#55266gaogaotiantian wants to merge 1 commit intoapache:masterfrom
Conversation
|
@jerrypeng as the original author. |
| self.broker = None | ||
|
|
||
| @classmethod | ||
| def has_dependencies(cls) -> bool: |
There was a problem hiding this comment.
can we add
have_testcontainers = have_package("testcontainers")
testcontainers_requirement_message = (
"" if have_testcontainers else "No module named 'testcontainers'"
)
have_kafka = have_package("kafka")
kafka_requirement_message = "" if have_kafka else "No module named 'kafka'"
in python/pyspark/testing/utils.py?
these deps were centrailized there
viirya
left a comment
viirya
left a comment
There was a problem hiding this comment.
I wonder should we install the dependency and let the pipeline run the tests instead of skipping them?
|
@gaogaotiantian I see the test is passing without problem for other PRs. Why is this run (https://github.com/apache/spark/actions/runs/24128422095) special that cause it to fail to due to missing dependencies? As @viirya mentioned, can we just install the dependencies instead of skipping the tests? |
|
What was the point of the other PR https://github.com/apache/spark/pull/55270/changes ? It didn't solve the issue? |
|
@jerrypeng @viirya
you can add the new dependencies in them if it makes sense and can make CI pass |
|
I'm okay to skip them on test pipelines with special purposes such as dependencies with old versions, etc. |
5 participants
What changes were proposed in this pull request?
has_dependenciestoKafkaUtilsos.environafter the test__main__instead of old entryWhy are the changes needed?
We don't want a test to fail if optional dependency is not available. It's breaking our CIs - https://github.com/apache/spark/actions/runs/24128422095 . The test itself should not have too much module-level code, and should have minimum side effect on the environment.
Does this PR introduce any user-facing change?
No.
How was this patch tested?
Confirmed it was correctly skipped when dependency was not available. CI should confirm whether test is still working properly.
Was this patch authored or co-authored using generative AI tooling?
No.