damccorm commented on code in PR #36561: URL: https://github.com/apache/beam/pull/36561#discussion_r2445169324
########## sdks/python/apache_beam/transforms/external_test.py: ########## @@ -66,6 +68,32 @@ # pylint: enable=wrong-import-order, wrong-import-position +def retry_on_grpc_timeout(max_retries=5, delay=10, max_total_time=300): Review Comment: I don't love the idea of forcing test retries since this will be equally true for customer jobs. Can we try the fix from @liferoad instead (https://github.com/apache/beam/pull/36528)? ########## sdks/python/test-suites/tox/common.gradle: ########## @@ -35,9 +35,7 @@ test.dependsOn "testPy${pythonVersionSuffix}Dill" // toxTask "testPy${pythonVersionSuffix}Dask", "py${pythonVersionSuffix}-dask", "${posargs}" // test.dependsOn "testPy${pythonVersionSuffix}Dask" project.tasks.register("preCommitPy${pythonVersionSuffix}") { - // Since codecoverage reports will always be generated for py39, - // all tests will be exercised. - // dependsOn = ["testPy${pythonVersionSuffix}Cloud", "testPython${pythonVersionSuffix}"] - dependsOn = ["testPy${pythonVersionSuffix}Cloud", "testPython${pythonVersionSuffix}"] + // PreCommit should only run non-cloud tests to avoid flakiness + dependsOn = ["testPython${pythonVersionSuffix}"] Review Comment: I think this will break our codecoverage reports. I'm supportive of the change if we can figure out how to keep our code coverage though (it is fine if this needs to be captured by a postcommit) ########## sdks/python/scripts/run_pytest.sh: ########## @@ -143,7 +143,8 @@ status2=$? # Exit with error if no tests were run in either suite (status code 5). if [[ $status1 == 5 && $status2 == 5 ]]; then - exit $status1 + echo "No tests were selected to run" + exit 0 Review Comment: I don't think we should make this change - this hides infrastructure issues. If there are calling sites that don't execute any tests, we should fix those instead ########## sdks/python/apache_beam/runners/portability/fn_api_runner/fn_runner.py: ########## @@ -416,6 +416,15 @@ def run_stages( stage_context (translations.TransformContext) stages (list[fn_api_runner.translations.Stage]) """ + # Apply environment configuration for stability + force_single_bundle = os.getenv('BEAM_TESTING_FORCE_SINGLE_BUNDLE', 'false').lower() == 'true' + deterministic_order = os.getenv('BEAM_TESTING_DETERMINISTIC_ORDER', 'false').lower() == 'true' Review Comment: Do these variables actually do anything other than log? -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
