All/task/devopsdsc 868 use work dir #530

matthijs-deltares · 2026-01-26T16:12:16Z

This PR introduces the "_work" copy of the test case input directory, which is used for test execution. This will be used to separate the work dir that executes the tests from the tracked dvc directory. The method of interest is __copy_to_work_folder. The rest are code improvements.

Copilot

Pull request overview

This PR introduces a "_work" copy mechanism for test case input directories, separating the working directory used for test execution from the tracked DVC directory. This prevents test execution from modifying tracked files.

Changes:

Added DirectoryState dataclass to capture directory snapshots using UTC timestamps instead of raw float values
Implemented __copy_to_work_folder method to create and manage work directory copies of input directories
Modified path handling to use "_work" suffixed paths for test execution

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
test/deltares_testbench/src/suite/test_case.py	Refactored directory state tracking with `DirectoryState` dataclass and UTC timestamps
test/deltares_testbench/src/suite/test_set_runner.py	Added work folder copy logic and updated path handling to use "_work" suffix
test/deltares_testbench/test/suite/test_TestCase.py	Added test for runfile changes detection
test/deltares_testbench/test/suite/test_ComparisonRunner.py	Added comprehensive tests for work folder creation, copying, and overwriting

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

test/deltares_testbench/src/suite/test_case.py

test/deltares_testbench/src/suite/test_set_runner.py

…irectory from the work directory

… accordingly

savente93

couple of questions and comments but nothing major

savente93 · 2026-01-27T09:30:23Z

test/deltares_testbench/src/suite/test_set_runner.py


+    def __copy_to_work_folder(self, local_path: str, logger: ILogger) -> None:
+        """Copy downloaded files to work folder if needed."""
+        copy_path = local_path + "_work"


Suggested change

copy_path = local_path + "_work"

copy_path = Path(local_path) / "_work"

savente93 · 2026-01-27T09:31:43Z

test/deltares_testbench/src/suite/test_set_runner.py

        """Set absolute paths on the config based on location type."""
        if location_type == PathType.INPUT:
-            config.absolute_test_case_path = local_path
+            config.absolute_test_case_path = local_path + "_work"


I'd advie to use Path here as well if possible

savente93 · 2026-01-27T09:34:44Z

test/deltares_testbench/src/suite/test_case.py

+@dataclass
+class DirectoryState:
+    """Snapshot of a directory's state used to detect added/changed files.
+
+    Attributes
+    ----------
+    files : Dict[str, datetime]
+        Mapping of filename to last modification time (UTC).
+    size : int
+        Total size in bytes of all files in the directory at snapshot time.
+    """
+
+    files: Dict[str, datetime] = field(default_factory=dict)
+    size: int = 0
+
+


I'm not sure I understand the point of this class. I see the data gets collected, but I don't see it being used anywhere. Could you explain the thought process behind it?

savente93 · 2026-01-27T09:37:58Z

test/deltares_testbench/test/suite/test_ComparisonRunner.py

+        )
+        logger = MagicMock(spec=ConsoleLogger)
+        runner = ComparisonRunner(settings, logger)
+        run_data = RunData(1, 10)


Suggested change

run_data = RunData(1, 10)

# get the data of test case nr 10

run_data = RunData(1, 10)

For someone unfamiliar with the code base (like me) I think this comment, or something like it would be very useful

savente93 · 2026-01-27T09:39:45Z

test/deltares_testbench/test/suite/test_ComparisonRunner.py

+        runner.run_test_case(config=config, run_data=run_data)
+
+        # Assert
+        assert fs.exists(expected_work_path)


if you want to be thurough you might want to test that the coppied location also has the correct files, but this might be more work than it is worth, so I'll leave it up to you whether you want to do this.

savente93 · 2026-01-27T09:40:28Z

test/deltares_testbench/test/suite/test_ComparisonRunner.py

+        run_data = RunData(1, 1)
+
+        expected_local_input_path = "/cases/win64/Name_1"
+        expected_work_path = expected_local_input_path + "_work"


especially if these are windows cases (as I assume from the win64 in the path) then it's important to use Path objects here

savente93 · 2026-01-27T09:41:48Z

test/deltares_testbench/test/suite/test_ComparisonRunner.py

+        expected_local_input_path = "/cases/win64/Name_1"
+        expected_work_path = expected_local_input_path + "_work"


Again, use Path objects here

savente93 · 2026-01-27T09:43:25Z

test/deltares_testbench/src/suite/test_set_runner.py

+            shutil.copytree(local_path, copy_path, symlinks=False, ignore_dangling_symlinks=True)
+        else:
+            os.makedirs(os.path.dirname(copy_path) or ".", exist_ok=True)
+            shutil.copy2(local_path, copy_path)


why copy2? what's the difference with copytree?

…viour is captured.

rene-deltares · 2026-01-28T14:54:57Z

test/deltares_testbench/src/suite/test_set_runner.py

+        logger.debug(f"Copying input from {local_path} to {copy_path}")
+        if os.path.isdir(local_path):
+            shutil.copytree(local_path, copy_path, symlinks=False, ignore_dangling_symlinks=True)
+        else:


I think we can rule out that local_path is not a directory, right? The input of a case should always be a directory. Maybe do a check at the start of this method: local_path.is_dir() and raise the builtin NotADirectoryError. Then you can skip this edgecase.

rene-deltares · 2026-01-28T14:59:39Z

test/deltares_testbench/src/suite/test_set_runner.py

                    raise TestBenchError(error_message) from e

+    def __copy_to_work_folder(self, local_path: str, logger: ILogger) -> None:
+        """Copy downloaded files to work folder if needed."""


I agree that local_path should be a Path instead of a str. What if there's a trailing slash in the string in some config files or something. Then you get <path>/_work, or maybe <path>\_work depending on the platform. The os.path functionality can all be replaced by Path methods. Path implements the PathLike protocol, which shutil.copytree accepts as arguments. So you don't even need to convert the Paths to str before passing them to shutil.copytree

rene-deltares · 2026-01-28T15:24:07Z

test/deltares_testbench/src/suite/test_set_runner.py

        """Set absolute paths on the config based on location type."""
        if location_type == PathType.INPUT:
-            config.absolute_test_case_path = local_path
+            config.absolute_test_case_path = local_path + "_work"


config.absolute_test_case_path is a str for historical reasons, right? I suggest something like this to avoid problems with path separators.

local = Path(local_path) config.absolute_test_case_path = str(local.parent / (local.name + "_work"))

matthijs-deltares force-pushed the all/task/DEVOPSDSC-868-use-work-dir branch from 5d0578d to b2c5f10 Compare January 26, 2026 16:18

matthijs-deltares requested a review from Copilot January 26, 2026 16:18

Copilot AI reviewed Jan 26, 2026

View reviewed changes

test/deltares_testbench/src/suite/test_case.py Outdated Show resolved Hide resolved

test/deltares_testbench/src/suite/test_set_runner.py Outdated Show resolved Hide resolved

matthijs-deltares requested review from rene-deltares and savente93 January 26, 2026 16:31

matthijs-deltares added 3 commits January 27, 2026 08:41

Use workdir as copy of the downloaded input to seperate the tracked d…

b8a4b1e

…irectory from the work directory

Enhance TestCase to track directory state changes and update run file…

829f6fd

… accordingly

Review comments and don't patch private methods

84a2ff8

matthijs-deltares force-pushed the all/task/DEVOPSDSC-868-use-work-dir branch from 8e3898e to 84a2ff8 Compare January 27, 2026 07:42

savente93 reviewed Jan 27, 2026

View reviewed changes

Add tests for the Paths class. Next step is refactoring when the beha…

0611086

…viour is captured.

matthijs-deltares force-pushed the all/task/DEVOPSDSC-868-use-work-dir branch from 6bd0885 to 0611086 Compare January 28, 2026 09:14

rene-deltares reviewed Jan 28, 2026

View reviewed changes

	copy_path = local_path + "_work"
	copy_path = Path(local_path) / "_work"

	run_data = RunData(1, 10)
	# get the data of test case nr 10
	run_data = RunData(1, 10)

		expected_local_input_path = "/cases/win64/Name_1"
		expected_work_path = expected_local_input_path + "_work"

All/task/devopsdsc 868 use work dir #530

Are you sure you want to change the base?

All/task/devopsdsc 868 use work dir #530

Uh oh!

Conversation

matthijs-deltares commented Jan 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

savente93 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

matthijs-deltares commented Jan 26, 2026 •

edited

Loading