dongjoon-hyun commented on code in PR #59250:
URL: https://github.com/apache/spark/pull/59250#discussion_r4427072111
##########
dev/sparktestsupport/utils.py:
##########
@@ -154,6 +156,27 @@ def determine_modules_to_test(changed_modules,
deduplicated=True):
)
+def determine_shell_script_test_goals(
+ test_modules: Sequence[modules.Module], stage: modules.TestStage
+) -> tuple[modules.ShellScriptTestGoal, ...]:
+ """
+ Return the shell script test goals that the given modules declare for a
stage.
+
+ Goals are returned in the order determined by the modules passed in,
+ preserving each module's declaration order. Each goal is returned only
once:
+
+ >>> from pathlib import Path
+ >>> goals = determine_shell_script_test_goals(
+ ... [modules.build, modules.root], modules.TestStage.PRE_BUILD)
+ >>> [g.path for g in goals].count(Path("dev/test-dependencies.sh"))
+ 1
+ """
+ declared_goals = itertools.chain.from_iterable(m.shell_script_test_goals
for m in test_modules)
+ stage_goals = (goal for goal in declared_goals if goal.stage == stage)
Review Comment:
**1. [Low] A plain-string `stage` silently runs no script**
`ShellScriptTestGoal` rejects a plain-string stage because it "would
silently never match", but the query side accepts one. `TestStage` is a plain
`Enum`, so `goal.stage == "post_build"` is always `False`. Then
`run_shell_script_tests(test_modules, "post_build")` runs nothing and prints
nothing, and the CI stays green.
Each new stage will add a call site like this one. Shall we apply the same
check here?
```python
if not isinstance(stage, modules.TestStage):
raise TypeError(f"Unsupported test stage: {stage!r}")
```
##########
dev/sparktestsupport/modules.py:
##########
@@ -102,6 +105,70 @@ 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 may be supplied as a `str` or `Path`, and is stored as a
normalized `Path`:
+
+ >>> goal.path == Path("dev/example")
+ True
+ >>> ShellScriptTestGoal(Path("dev/example")) == goal
+ True
+ >>> ShellScriptTestGoal("./dev//example") == goal
+ True
+ >>> len({goal, ShellScriptTestGoal("./dev//example"),
+ ... ShellScriptTestGoal(Path("dev/example"))})
+ 1
+
+ The path is a non-blank, repository-relative script:
+
+ >>> ShellScriptTestGoal("/tmp/example")
+ Traceback (most recent call last):
+ ...
+ ValueError: Shell script path must be relative to repo root: '/tmp/example'
+ >>> ShellScriptTestGoal(" ")
Review Comment:
**5. [Low] No doctest covers the `..` and empty path rejections**
The examples cover the absolute and blank paths, but not the `".." in
path.parts` and `path == Path()` conditions, so removing either one still
passes. `..` matters the most. `os.path.join(SPARK_HOME, "../x")` points
outside the repository, and the `is_exe` doctest passes as long as that file
exists.
Shall we add examples for `"../x"` and `""` here?
##########
dev/run-tests.py:
##########
@@ -602,9 +618,10 @@ 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(
Review Comment:
**2. [Low] No CI job runs a script through this call**
All five `dev/run-tests` calls in the workflows (`build_and_test.yml` L402,
L678, L797, L1163 and `python_hosted_runner_test.yml` L207) pass `--modules`,
and none of their module lists has `build` or `root`. So, CI only reaches this
call with an empty goal list, and the loop body never runs. The dependency
check in CI is still the direct `./dev/test-dependencies.sh` step at
`build_and_test.yml` L880.
The new doctest in `run_shell_script_tests` doesn't call the function
either. So, the green CI doesn't verify the migrated path, and a break in it
would show up only in a local run that selects `build` or `root`.
Could we add the result of such a manual run to `How was this patch
tested?`, or a doctest with a stubbed `run_cmd`? That section also mentions
only `sparktestsupport`, while this PR adds a doctest to `dev/run-tests.py` too.
##########
dev/sparktestsupport/modules.py:
##########
@@ -102,6 +105,70 @@ 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 may be supplied as a `str` or `Path`, and is stored as a
normalized `Path`:
+
+ >>> goal.path == Path("dev/example")
+ True
+ >>> ShellScriptTestGoal(Path("dev/example")) == goal
+ True
+ >>> ShellScriptTestGoal("./dev//example") == goal
+ True
+ >>> len({goal, ShellScriptTestGoal("./dev//example"),
+ ... ShellScriptTestGoal(Path("dev/example"))})
+ 1
+
+ The path is a non-blank, repository-relative script:
+
+ >>> ShellScriptTestGoal("/tmp/example")
+ Traceback (most recent call last):
+ ...
+ ValueError: Shell script path must be relative to repo root: '/tmp/example'
+ >>> ShellScriptTestGoal(" ")
+ Traceback (most recent call last):
+ ...
+ ValueError: Shell script path must be relative to repo root: ' '
+ """
+
+ path: Path
+ stage: TestStage = TestStage.PRE_BUILD
+
+ def __init__(self, path: str | Path, stage: TestStage =
TestStage.PRE_BUILD) -> None:
+ if not isinstance(stage, TestStage):
+ raise TypeError(f"Unsupported test stage: {stage!r}")
+ # `Path` rejects non-path types and normalizes spellings like
"./dev//x".
+ path = Path(path)
+ if not str(path).strip() or path == Path() or path.is_absolute() or
".." in path.parts:
+ raise ValueError(f"Shell script path must be relative to repo
root: {str(path)!r}")
Review Comment:
**6. [Nit] The `ValueError` message can show the wrong input and reason**
It prints the normalized path and gives the same reason for every rejected
case.
- `ShellScriptTestGoal("")` reports `'.'`, which the caller didn't write.
- `ShellScriptTestGoal("../x")` says the path must be relative, although it
already is.
Shall we print the original value and say something like `must be a relative
path inside the repository`?
##########
dev/sparktestsupport/utils.py:
##########
@@ -154,6 +156,27 @@ def determine_modules_to_test(changed_modules,
deduplicated=True):
)
+def determine_shell_script_test_goals(
+ test_modules: Sequence[modules.Module], stage: modules.TestStage
+) -> tuple[modules.ShellScriptTestGoal, ...]:
+ """
+ Return the shell script test goals that the given modules declare for a
stage.
+
+ Goals are returned in the order determined by the modules passed in,
+ preserving each module's declaration order. Each goal is returned only
once:
+
+ >>> from pathlib import Path
+ >>> goals = determine_shell_script_test_goals(
+ ... [modules.build, modules.root], modules.TestStage.PRE_BUILD)
+ >>> [g.path for g in goals].count(Path("dev/test-dependencies.sh"))
Review Comment:
**4. [Low] This doctest has no case of a module that selects nothing**
It only counts the goals for `[build, root]`, so it still passes if the
function ignores `test_modules` and always returns the goals of `root`. In that
case, every `--modules` run would execute `dev/test-dependencies.sh`, which
`should_run_build_tests` used to limit to `build` and `root`.
How about adding the negative case? The order stated in the sentence above
has no example either.
```python
>>> determine_shell_script_test_goals([modules.graphx],
modules.TestStage.PRE_BUILD)
()
```
##########
dev/sparktestsupport/modules.py:
##########
@@ -102,6 +105,70 @@ 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 may be supplied as a `str` or `Path`, and is stored as a
normalized `Path`:
+
+ >>> goal.path == Path("dev/example")
+ True
+ >>> ShellScriptTestGoal(Path("dev/example")) == goal
+ True
+ >>> ShellScriptTestGoal("./dev//example") == goal
+ True
+ >>> len({goal, ShellScriptTestGoal("./dev//example"),
+ ... ShellScriptTestGoal(Path("dev/example"))})
+ 1
+
+ The path is a non-blank, repository-relative script:
+
+ >>> ShellScriptTestGoal("/tmp/example")
+ Traceback (most recent call last):
+ ...
+ ValueError: Shell script path must be relative to repo root: '/tmp/example'
+ >>> ShellScriptTestGoal(" ")
+ Traceback (most recent call last):
+ ...
+ ValueError: Shell script path must be relative to repo root: ' '
+ """
+
+ path: Path
+ stage: TestStage = TestStage.PRE_BUILD
Review Comment:
**8. [Nit] The `stage` default is declared twice**
Since this class defines `__init__`, `@dataclass` doesn't generate one, and
this default isn't used to create objects. If only one of this line and L160
changes later, `ShellScriptTestGoal.stage` and `dataclasses.fields()` will
disagree with the real default.
Shall we declare just `stage: TestStage` here?
##########
dev/run-tests.py:
##########
@@ -380,8 +381,23 @@ 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):
+ """
+ Run the selected shell script goals for the given stage.
+
+ All declared scripts must exist and be executable before the build:
+
+ >>> from sparktestsupport.shellutils import is_exe
+ >>> [str(g.path) for g in modules.root.shell_script_test_goals
Review Comment:
**3. [Low] The `is_exe` doctest checks only the goals of `root`**
The text says `All declared scripts`, but
`modules.root.shell_script_test_goals` is a snapshot taken at import time. A
script of a module declared below `root` is not checked. This is the same cause
as [the earlier snapshot
comment](https://github.com/apache/spark/pull/59250#discussion_r4299939068),
and it's not a problem today because `root` is defined last.
This is the form I suggested in the previous round, but
`modules.all_modules` would be better because it has no such gap.
```python
>>> [str(g.path) for m in modules.all_modules for g in
m.shell_script_test_goals
... if not is_exe(os.path.join(SPARK_HOME, g.path))]
[]
```
##########
dev/sparktestsupport/modules.py:
##########
@@ -102,6 +105,70 @@ 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 may be supplied as a `str` or `Path`, and is stored as a
normalized `Path`:
+
+ >>> goal.path == Path("dev/example")
+ True
+ >>> ShellScriptTestGoal(Path("dev/example")) == goal
+ True
+ >>> ShellScriptTestGoal("./dev//example") == goal
+ True
+ >>> len({goal, ShellScriptTestGoal("./dev//example"),
+ ... ShellScriptTestGoal(Path("dev/example"))})
+ 1
+
+ The path is a non-blank, repository-relative script:
+
+ >>> ShellScriptTestGoal("/tmp/example")
+ Traceback (most recent call last):
+ ...
+ ValueError: Shell script path must be relative to repo root: '/tmp/example'
+ >>> ShellScriptTestGoal(" ")
+ Traceback (most recent call last):
+ ...
+ ValueError: Shell script path must be relative to repo root: ' '
+ """
+
+ path: Path
+ stage: TestStage = TestStage.PRE_BUILD
+
+ def __init__(self, path: str | Path, stage: TestStage =
TestStage.PRE_BUILD) -> None:
+ if not isinstance(stage, TestStage):
+ raise TypeError(f"Unsupported test stage: {stage!r}")
+ # `Path` rejects non-path types and normalizes spellings like
"./dev//x".
+ path = Path(path)
+ if not str(path).strip() or path == Path() or path.is_absolute() or
".." in path.parts:
Review Comment:
**7. [Nit] Whitespace around a path part is accepted**
`str(path).strip()` catches only a path that is entirely blank, so `"
dev/x"`, `"dev/x "` and `" / "` pass, although the docstring says `non-blank`.
Also, `ShellScriptTestGoal(" dev/x")` isn't equal to
`ShellScriptTestGoal("dev/x")`, so `dict.fromkeys` keeps both.
Checking each part would also cover the blank case.
```python
any(part != part.strip() for part in path.parts)
```
##########
dev/sparktestsupport/modules.py:
##########
@@ -141,23 +208,51 @@ 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 sequence (e.g., a list or tuple) of
shell script test goals
Review Comment:
**9. [Nit] This line has 101 characters**
It's one over `line-length = 100` in `pyproject.toml`. `ruff check` doesn't
report it because `E501` isn't selected. Shall we move `goals` to the next line?
--
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]
