[GitHub] [spark] HyukjinKwon commented on a change in pull request #33125: [SPARK-35483][TESTS] Add sql dependency to docker_integration_tests
HyukjinKwon commented on a change in pull request #33125: URL: https://github.com/apache/spark/pull/33125#discussion_r660295086 ## File path: dev/sparktestsupport/modules.py ## @@ -769,7 +769,7 @@ def __hash__(self): docker_integration_tests = Module( name="docker-integration-tests", -dependencies=[], +dependencies=[sql], Review comment: hm, the problem is that the tests will still be skipped because `ENABLE_DOCKER_INTEGRATION_TESTS` is set only when explicitly there are some changes in `docker_integration_tests` module alone. (e.g., when `sql` module is picked alone, `ENABLE_DOCKER_INTEGRATION_TESTS` is not set, and the related tests are skipped). This logic was followed from the Kinesis one but maybe we should just remove `ENABLE_DOCKER_INTEGRATION_TESTS` away and treat it like other test cases. This env approach was used because of the concern of flakiness IIRC but actually the test seems pretty stable (?). ## File path: dev/sparktestsupport/modules.py ## @@ -769,7 +769,7 @@ def __hash__(self): docker_integration_tests = Module( name="docker-integration-tests", -dependencies=[], +dependencies=[sql], Review comment: alternatively we can copy: ``` environ=None if "GITHUB_ACTIONS" not in os.environ else { "ENABLE_DOCKER_INTEGRATION_TESTS": "1" }, ``` to `sql` module too to be extra conservative. ## File path: dev/sparktestsupport/modules.py ## @@ -769,7 +769,7 @@ def __hash__(self): docker_integration_tests = Module( name="docker-integration-tests", -dependencies=[], +dependencies=[sql], Review comment: Yeah, that should be fine for now 👍 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a change in pull request #33125: [SPARK-35483][TESTS] Add sql dependency to docker_integration_tests
HyukjinKwon commented on a change in pull request #33125: URL: https://github.com/apache/spark/pull/33125#discussion_r660297375 ## File path: dev/sparktestsupport/modules.py ## @@ -769,7 +769,7 @@ def __hash__(self): docker_integration_tests = Module( name="docker-integration-tests", -dependencies=[], +dependencies=[sql], Review comment: Yeah, that should be fine for now 👍 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a change in pull request #33125: [SPARK-35483][TESTS] Add sql dependency to docker_integration_tests
HyukjinKwon commented on a change in pull request #33125: URL: https://github.com/apache/spark/pull/33125#discussion_r660295439 ## File path: dev/sparktestsupport/modules.py ## @@ -769,7 +769,7 @@ def __hash__(self): docker_integration_tests = Module( name="docker-integration-tests", -dependencies=[], +dependencies=[sql], Review comment: alternatively we can copy: ``` environ=None if "GITHUB_ACTIONS" not in os.environ else { "ENABLE_DOCKER_INTEGRATION_TESTS": "1" }, ``` to `sql` module too to be extra conservative. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] [spark] HyukjinKwon commented on a change in pull request #33125: [SPARK-35483][TESTS] Add sql dependency to docker_integration_tests
HyukjinKwon commented on a change in pull request #33125: URL: https://github.com/apache/spark/pull/33125#discussion_r660295086 ## File path: dev/sparktestsupport/modules.py ## @@ -769,7 +769,7 @@ def __hash__(self): docker_integration_tests = Module( name="docker-integration-tests", -dependencies=[], +dependencies=[sql], Review comment: hm, the problem is that the tests will still be skipped because `ENABLE_DOCKER_INTEGRATION_TESTS` is set only when explicitly there are some changes in `docker_integration_tests` module alone. (e.g., when `sql` module is picked alone, `ENABLE_DOCKER_INTEGRATION_TESTS` is not set, and the related tests are skipped). This logic was followed from the Kinesis one but maybe we should just remove `ENABLE_DOCKER_INTEGRATION_TESTS` away and treat it like other test cases. This env approach was used because of the concern of flakiness IIRC but actually the test seems pretty stable (?). -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org