dongjoon-hyun commented on code in PR #59250:
URL: https://github.com/apache/spark/pull/59250#discussion_r4210687396
##########
dev/sparktestsupport/utils.py:
##########
@@ -154,6 +163,93 @@ def determine_modules_to_test(changed_modules,
deduplicated=True):
)
+def determine_test_goals_for_files(filenames: Iterable[str]) -> TestGoals:
+ """
+ Return the module-declared test goals selected for a set of changed files.
+
+ A Spark Connect server change also selects the PySpark Connect tests that
depend on it:
+
+ >>> goals =
determine_test_goals_for_files(["sql/connect/server/src/main/scala/Service.scala"])
+ >>> goals.sbt
Review Comment:
This doctest pins the exact `sbt_test_goals` of the `connect` module (and,
below, specific PySpark error test names). Since `dev/run-tests.py` runs
`_test()` before `main()`, an unrelated edit to `modules.py` (e.g., adding a
goal to `connect`) would make every `./dev/run-tests` invocation exit with `-1`
before running any test.
Could we use synthetic modules (like the `SimpleNamespace` approach in
`determine_shell_script_test_goals`) or assert only on the goals this PR
introduces?
##########
dev/sparktestsupport/utils.py:
##########
@@ -154,6 +163,93 @@ def determine_modules_to_test(changed_modules,
deduplicated=True):
)
+def determine_test_goals_for_files(filenames: Iterable[str]) -> TestGoals:
+ """
+ Return the module-declared test goals selected for a set of changed files.
+
+ A Spark Connect server change also selects the PySpark Connect tests that
depend on it:
+
+ >>> goals =
determine_test_goals_for_files(["sql/connect/server/src/main/scala/Service.scala"])
+ >>> goals.sbt
+ ('connect/test', 'connect-client-jvm/test', 'connect-client-jdbc/test')
+ >>> "pyspark.sql.tests.connect.test_connect_basic" in goals.python
+ True
+ >>> goals.shell_script
+ ()
+
+ A PySpark error change includes its error-specific Python tests:
+
+ >>> goals =
determine_test_goals_for_files(["python/pyspark/errors/error.py"])
+ >>> goals.sbt
+ ()
+ >>> {
+ ... "pyspark.errors.tests.test_connect_errors_conversion",
+ ... "pyspark.errors.tests.test_errors",
+ ... "pyspark.errors.tests.test_traceback",
+ ... "pyspark.errors.tests.connect.test_parity_traceback",
+ ... } <= set(goals.python)
+ True
+
+ A POM change selects the dependency check:
+
+ >>> goals = determine_test_goals_for_files(["pom.xml"])
+ >>> "dev/test-dependencies.sh" in [g.path for g in goals.shell_script]
+ True
+
+ Ignored files select no module goals:
+
+ >>> goals = determine_test_goals_for_files(["python/README.md"])
+ >>> (goals.sbt, goals.python, goals.shell_script)
+ ((), (), ())
+
+ An otherwise unmatched path selects the root module:
+
+ >>> goals = determine_test_goals_for_files(["new-file.txt"])
+ >>> goals == TestGoals(
+ ... sbt=tuple(modules.root.sbt_test_goals),
+ ... python=tuple(modules.root.python_test_goals),
+ ... shell_script=tuple(modules.root.shell_script_test_goals),
+ ... )
+ True
+ """
+ test_modules =
determine_modules_to_test(determine_modules_for_files(filenames))
+ return TestGoals(
+ sbt=tuple(itertools.chain.from_iterable(m.sbt_test_goals for m in
test_modules)),
+ python=tuple(itertools.chain.from_iterable(m.python_test_goals for m
in test_modules)),
+ shell_script=tuple(
Review Comment:
Unlike `determine_shell_script_test_goals`, this neither filters by stage
nor deduplicates. It can return the same goal twice and mix goals of different
stages, so it disagrees with what `run-tests.py` actually executes. If we keep
this function, it would be better to reuse the same deduplication logic.
##########
dev/run-tests.py:
##########
@@ -622,13 +638,22 @@ def main():
if os.environ.get("SKIP_SCALA_BUILD", "false") != "true":
with titled_block("Building Spark assembly"):
build_spark_assembly_sbt(extra_profiles)
+ run_shell_script_tests(
+ test_modules,
+ modules.TestStage.POST_ASSEMBLY_SBT,
Review Comment:
Since `build_tool` is hard-coded to `"sbt"`, the only real difference
between `POST_ASSEMBLY_SBT` and `POST_ASSEMBLY` is whether the stage is skipped
under `SKIP_SCALA_BUILD=true`. The `_SBT` suffix doesn't convey that. A name
reflecting the actual gating would help script authors choose the correct stage.
##########
dev/run-tests.py:
##########
@@ -380,8 +381,17 @@ def run_python_packaging_tests():
run_cmd(command)
-def run_build_tests():
- run_cmd([os.path.join(SPARK_HOME, "dev", "test-dependencies.sh")])
+def run_shell_script_tests(test_modules, stage):
+ shell_script_test_goals = determine_shell_script_test_goals(test_modules,
stage)
+
+ if not shell_script_test_goals:
+ return
+
+ title = f"Running {stage.value} shell script tests"
Review Comment:
Previously, this block was titled `Running build tests`. Once several
scripts share a stage, all of them will be grouped under one generic `Running
pre_build shell script tests` group in the GitHub Actions log. What about one
`titled_block` per script (e.g., titled with `test_goal.path`) so that a
failure is easy to locate?
##########
dev/sparktestsupport/utils.py:
##########
@@ -154,6 +163,93 @@ def determine_modules_to_test(changed_modules,
deduplicated=True):
)
+def determine_test_goals_for_files(filenames: Iterable[str]) -> TestGoals:
Review Comment:
`TestGoals` and `determine_test_goals_for_files` don't seem to have any
caller except their own doctests, and the shell script test goal feature
doesn't need them. Shall we remove them from this PR and add them together with
an actual user?
##########
dev/sparktestsupport/modules.py:
##########
@@ -166,10 +208,10 @@ def __init__(
self.build_profile_flags = build_profile_flags
self.environ = environ or {}
self.python_test_goals = python_test_goals
+ self.shell_script_test_goals = shell_script_test_goals
Review Comment:
`ShellScriptTestGoal.__post_init__` rejects a plain-string stage, but
nothing rejects a plain-string goal, e.g.,
`shell_script_test_goals=["dev/lint-java"]`. The module definition succeeds,
and the failure only surfaces as `AttributeError: 'str' object has no attribute
'stage'` inside `determine_shell_script_test_goals` after the RAT and style
checks have already run. Via `root`, the error doesn't point at the offending
module either. Could we validate the element type here?
##########
dev/sparktestsupport/modules.py:
##########
@@ -102,6 +104,46 @@ def is_ignored_file(filename: str) -> bool:
return False
+class TestStage(Enum):
+ """
+ When a test goal runs in dev/run-tests.py, relative to the Spark build.
+ """
+
+ PRE_BUILD = "pre_build"
+ # Before assembly; still runs when SKIP_SCALA_BUILD=true.
+ POST_BUILD = "post_build"
Review Comment:
A stage only encodes the position relative to the build. Some of the planned
migrations depend on additional conditions in `run-tests.py`: `dev/mima` is
guarded by `SKIP_MIMA` (set in many GitHub Actions jobs), and the Java style
check by the changed files. `run_shell_script_tests` has no way to honor these,
so we may end up adding per-script special cases back. It might be worth
deciding how a goal expresses such conditions before fixing the list of stages.
##########
dev/run-tests.py:
##########
@@ -602,15 +612,21 @@ def main():
with titled_block("Running R style checks"):
run_sparkr_style_checks()
- if any(m.should_run_build_tests for m in test_modules):
- with titled_block("Running build tests"):
- run_build_tests()
+ run_shell_script_tests(
+ test_modules,
+ modules.TestStage.PRE_BUILD,
+ )
# spark build
if os.environ.get("SKIP_SCALA_BUILD", "false") != "true":
with titled_block("Building Spark"):
build_apache_spark(build_tool, extra_profiles)
+ run_shell_script_tests(
+ test_modules,
+ modules.TestStage.POST_BUILD,
Review Comment:
In this PR, only `PRE_BUILD` has a goal, and `POST_BUILD`,
`POST_ASSEMBLY_SBT`, and `POST_ASSEMBLY` are unused. Could we add each stage
(and its call site) in the PR which migrates the first script into it? Then the
insertion point can be reviewed together with its actual user.
##########
dev/sparktestsupport/modules.py:
##########
@@ -102,6 +104,46 @@ def is_ignored_file(filename: str) -> bool:
return False
+class TestStage(Enum):
+ """
+ When a test goal runs in dev/run-tests.py, relative to the Spark build.
+ """
+
+ PRE_BUILD = "pre_build"
+ # Before assembly; still runs when SKIP_SCALA_BUILD=true.
+ POST_BUILD = "post_build"
+ # After SBT assembly; skipped when SKIP_SCALA_BUILD=true.
+ POST_ASSEMBLY_SBT = "post_assembly_sbt"
+ # After assembly, whether built from scratch or restored from an artifact;
+ # still runs when SKIP_SCALA_BUILD=true.
+ POST_ASSEMBLY = "post_assembly"
+
+
+@dataclass(frozen=True)
+class ShellScriptTestGoal:
+ """
+ Describe a shell script test goal.
+
+ >>> goal = ShellScriptTestGoal("dev/example")
+ >>> goal.stage.value
+ 'pre_build'
+
+ A plain-string stage would silently never match, so it is rejected:
+
+ >>> ShellScriptTestGoal("dev/example", "post_build")
+ Traceback (most recent call last):
+ ...
+ TypeError: Unsupported test stage: 'post_build'
+ """
+
+ path: str
+ stage: TestStage = TestStage.PRE_BUILD
+
+ def __post_init__(self) -> None:
Review Comment:
`path` is not validated. For an absolute path, `os.path.join(SPARK_HOME,
test_goal.path)` in `run-tests.py` silently discards `SPARK_HOME`. Shall we
also check that `path` is a non-empty relative path here?
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
