dongjoon-hyun commented on code in PR #59250:
URL: https://github.com/apache/spark/pull/59250#discussion_r4299939039
##########
dev/sparktestsupport/modules.py:
##########
@@ -141,23 +187,33 @@ def __init__(
module are changed.
:param sbt_test_goals: A set of SBT test goals for testing this module.
:param python_test_goals: A set of Python test goals for testing this
module.
+ :param shell_script_test_goals: A set of shell script test goals for
testing this module.
:param excluded_python_implementations: A set of Python
implementations that are not
supported by this module's Python components. The values in this
set should match
strings returned by Python's `platform.python_implementation()`.
:param test_tags A set of tags that will be excluded when running unit
tests if the module
is not explicitly changed.
:param should_run_r_tests: If true, changes in this module will
trigger all R tests.
- :param should_run_build_tests: If true, changes in this module will
trigger build tests.
Duplicate names are rejected and not registered:
>>> Module("core", [], [])
Traceback (most recent call last):
...
ValueError: module name 'core' is already in use
+
+ A plain-string goal fails here, not later as an AttributeError:
+
+ >>> Module("bad-goal", [], [],
shell_script_test_goals=["dev/lint-java"])
+ Traceback (most recent call last):
+ ...
+ TypeError: bad-goal: expected ShellScriptTestGoal, got 'dev/lint-java'
"""
if any(module.name == name for module in all_modules):
raise ValueError(f"module name {name!r} is already in use")
+ for goal in shell_script_test_goals:
Review Comment:
This loop iterates `shell_script_test_goals` before it is stored at L225, so
a generator argument is exhausted by the validation and the module silently
ends up with no goals.
```python
>>> goals = (ShellScriptTestGoal(p) for p in ["dev/a", "dev/b"])
>>> m = Module("gen-mod", [], [], shell_script_test_goals=goals)
>>> list(m.shell_script_test_goals)
[]
```
Then none of the scripts run and CI stays green. Shall we materialize it
once, e.g., `shell_script_test_goals = tuple(shell_script_test_goals)`, before
the validation?
##########
dev/sparktestsupport/utils.py:
##########
@@ -154,6 +156,36 @@ def determine_modules_to_test(changed_modules,
deduplicated=True):
)
+def determine_shell_script_test_goals(
+ test_modules: Iterable[modules.Module], stage: modules.TestStage
+) -> tuple[modules.ShellScriptTestGoal, ...]:
+ """
+ Return the shell script test goals that the given modules declare for a
stage.
+
+ Modules can declare the same goal, so each goal is returned once, in
declaration order:
Review Comment:
`in declaration order` holds only when `test_modules` itself is in
declaration order. In `dev/run-tests.py`, the GitHub Actions path builds it
with `test_modules = list(set(modules_to_test).intersection(test_modules))`,
and `Module.__hash__` is `hash(self.name)`, which is randomized per process
(`PYTHONHASHSEED` is not set). So, once two or more modules declare pre-build
scripts (e.g., `dev/lint-java` and `dev/lint-scala` in the planned migration),
their execution order can change from run to run.
Shall we collect the goals in a deterministic module order (e.g., the
`modules.all_modules` order), or adjust this sentence?
##########
dev/sparktestsupport/modules.py:
##########
@@ -102,6 +104,50 @@ def is_ignored_file(filename: str) -> bool:
return False
+class TestStage(Enum):
+ """
+ When a shell script test goal runs in dev/run-tests.py.
+ """
+
+ # Before the Spark build.
+ PRE_BUILD = "pre_build"
+
+
+@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'
+
+ The path is a repository-relative script:
+
+ >>> ShellScriptTestGoal("/tmp/example")
+ Traceback (most recent call last):
+ ...
+ ValueError: Shell script path must be relative to repo root: '/tmp/example'
+ """
+
+ path: str
+ stage: TestStage = TestStage.PRE_BUILD
+
+ def __post_init__(self) -> None:
+ if not isinstance(self.stage, TestStage):
+ raise TypeError(f"Unsupported test stage: {self.stage!r}")
+ parts = PurePath(self.path).parts
+ if not self.path or not self.path.strip() or os.path.isabs(self.path)
or ".." in parts:
Review Comment:
`path` is validated but not normalized. So, `ShellScriptTestGoal("dev/x")`
and `ShellScriptTestGoal("./dev/x")` are different goals, and `dict.fromkeys`
in `determine_shell_script_test_goals` doesn't deduplicate them. The same
script runs twice when both are selected (e.g., under `root`).
In addition, a non-string path fails with a different error from the
intended `ValueError`.
```python
>>> ShellScriptTestGoal(Path("dev/x"))
AttributeError: 'PosixPath' object has no attribute 'strip'
```
What about checking `isinstance(self.path, str)` and storing the normalized
form, e.g., `object.__setattr__(self, "path", PurePath(self.path).as_posix())`?
##########
dev/sparktestsupport/modules.py:
##########
@@ -141,23 +187,33 @@ def __init__(
module are changed.
:param sbt_test_goals: A set of SBT test goals for testing this module.
:param python_test_goals: A set of Python test goals for testing this
module.
+ :param shell_script_test_goals: A set of shell script test goals for
testing this module.
:param excluded_python_implementations: A set of Python
implementations that are not
supported by this module's Python components. The values in this
set should match
strings returned by Python's `platform.python_implementation()`.
:param test_tags A set of tags that will be excluded when running unit
tests if the module
is not explicitly changed.
:param should_run_r_tests: If true, changes in this module will
trigger all R tests.
- :param should_run_build_tests: If true, changes in this module will
trigger build tests.
Duplicate names are rejected and not registered:
>>> Module("core", [], [])
Traceback (most recent call last):
...
ValueError: module name 'core' is already in use
+
+ A plain-string goal fails here, not later as an AttributeError:
+
+ >>> Module("bad-goal", [], [],
shell_script_test_goals=["dev/lint-java"])
+ Traceback (most recent call last):
+ ...
+ TypeError: bad-goal: expected ShellScriptTestGoal, got 'dev/lint-java'
"""
if any(module.name == name for module in all_modules):
raise ValueError(f"module name {name!r} is already in use")
+ for goal in shell_script_test_goals:
+ if not isinstance(goal, ShellScriptTestGoal):
+ raise TypeError(f"{name}: expected ShellScriptTestGoal, got
{goal!r}")
Review Comment:
If a single string is passed instead of a list, e.g.,
`shell_script_test_goals="dev/lint-java"`, this loop iterates its characters
and reports the following.
```
TypeError: str-mod: expected ShellScriptTestGoal, got 'd'
```
It fails as intended, but the message doesn't point to the actual mistake.
Shall we reject a `str` (or a single `ShellScriptTestGoal`) explicitly before
the loop?
##########
dev/sparktestsupport/modules.py:
##########
@@ -1850,8 +1906,10 @@ def __hash__(self):
"test",
],
python_test_goals=list(itertools.chain.from_iterable(m.python_test_goals
for m in all_modules)),
+ shell_script_test_goals=list(
Review Comment:
Minor. Like `python_test_goals` above, this snapshots the goals of the
modules defined so far at import time. It's fine today because `root` is
defined last. However, a module added below `root` would be silently missing
from full (`root`) runs, while its script still runs when that module is
changed directly.
If we want to avoid this, `determine_shell_script_test_goals` could expand
`root` to `modules.all_modules` instead. Not a blocker.
##########
dev/sparktestsupport/utils.py:
##########
@@ -154,6 +156,36 @@ def determine_modules_to_test(changed_modules,
deduplicated=True):
)
+def determine_shell_script_test_goals(
+ test_modules: Iterable[modules.Module], stage: modules.TestStage
+) -> tuple[modules.ShellScriptTestGoal, ...]:
+ """
+ Return the shell script test goals that the given modules declare for a
stage.
+
+ Modules can declare the same goal, so each goal is returned once, in
declaration order:
+
+ >>> from types import SimpleNamespace
+ >>> Goal, Stage = modules.ShellScriptTestGoal, modules.TestStage
+ >>> shared = Goal("dev/shared")
+ >>> first = SimpleNamespace(shell_script_test_goals=[shared,
Goal("dev/first")])
+ >>> second = SimpleNamespace(shell_script_test_goals=[Goal("dev/second"),
shared])
+ >>> [g.path for g in determine_shell_script_test_goals([first, second],
Stage.PRE_BUILD)]
+ ['dev/shared', 'dev/first', 'dev/second']
Review Comment:
These examples use `SimpleNamespace` stand-ins only, so nothing checks the
actual migration of this PR, i.e., that the real modules still select
`dev/test-dependencies.sh`. If the `build` goal or the `root` aggregation is
broken, this function returns `()` and `run-tests` silently stops running
`test-dependencies.sh`.
Could we add a doctest with the real modules? To avoid pinning the full goal
list (which would break every `run-tests` invocation whenever another script is
added), a membership check would be enough.
```python
>>> "dev/test-dependencies.sh" in [
... g.path for g in determine_shell_script_test_goals([modules.root],
Stage.PRE_BUILD)]
True
```
##########
dev/sparktestsupport/utils.py:
##########
@@ -154,6 +156,36 @@ def determine_modules_to_test(changed_modules,
deduplicated=True):
)
+def determine_shell_script_test_goals(
+ test_modules: Iterable[modules.Module], stage: modules.TestStage
+) -> tuple[modules.ShellScriptTestGoal, ...]:
+ """
+ Return the shell script test goals that the given modules declare for a
stage.
+
+ Modules can declare the same goal, so each goal is returned once, in
declaration order:
+
+ >>> from types import SimpleNamespace
+ >>> Goal, Stage = modules.ShellScriptTestGoal, modules.TestStage
+ >>> shared = Goal("dev/shared")
+ >>> first = SimpleNamespace(shell_script_test_goals=[shared,
Goal("dev/first")])
+ >>> second = SimpleNamespace(shell_script_test_goals=[Goal("dev/second"),
shared])
+ >>> [g.path for g in determine_shell_script_test_goals([first, second],
Stage.PRE_BUILD)]
+ ['dev/shared', 'dev/first', 'dev/second']
+
+ Goals for another stage are left out. The stand-in is not a
ShellScriptTestGoal
+ because that type only accepts a stage defined on the enum:
+
+ >>> other = SimpleNamespace(path="dev/later", stage="other")
Review Comment:
Since `TestStage` has only `PRE_BUILD`, a real `ShellScriptTestGoal` can
never be filtered out here. That's why this case needs a stand-in which the
real type rejects (`stage="other"`). It doesn't test a reachable state, and it
couples the doctest to the attribute names.
Shall we drop this case here and add the stage-filter example together with
the second `TestStage` member?
--
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]
