Test: test for `copy_examples` #60

cadenmyers13 · 2025-10-13T16:00:38Z

This test runs 70 tests. This breakdown is from the following 14 copy scenarios. Each copy scenario is written for each caseX generated in the temp dir. we have 5 cases so 14 * 5 = 70

# The copy scenarios are:
# a) copy to cwd
# b) copy to target dir

# 1) copy one example
# 2) copy list of examples from same pack
# 3) copy list of examples from different packs
# 4) copy all examples from a pack
# 5) copy all examples from list of packs
# 6) copy all examples from all packs

cadenmyers13 · 2025-10-13T16:06:19Z

@sbillinge ready for review

I recognize that the code for this test is somewhat particular because of the temp dir structures we are testing, but I think it should be robust to the UCs listed above. I wanted to reuse the same temp dir that we created for the other test so that this test casts a broader net of cases it can check. However, we could simplify this test if you think this might be a bit overkill.

tests/test_cli.py

sbillinge

I reviewed the comments but not the code yet. I can do that later but I don't see the function signature and am on my phone so will do it later

tests/test_cli.py

cadenmyers13 · 2025-10-13T19:42:14Z

@sbillinge So I dont override these changes before your review, here's a simpler test i wrote.

@pytest.mark.parametrize(
    "path",
    [None, Path("target/path")],
)
@pytest.mark.parametrize(
    "input,expected",
    [
        ("case1", [Path("")]),
        ("case2", [Path("ex1/solution/diffpy-cmi/script1.py"), Path("ex2/random/path/script1.py"), Path("ex2/random/path/script2.py")]),
        ("case3", [Path("ex1/script1.py"), Path("ex2/solutions/script1.py")]),
        ("case4", [Path("")]),
        ("case5", [Path("ex1/path1/script1.py"), Path("ex1/path2/script2.py")]),
    ]
)
def test_copy_examples(input, path, expected, example_cases):
    case_dir = example_cases / input
    if path is None:
        target_dir = None
    else:
        target_dir = case_dir / path
        target_dir.mkdir(parents=True, exist_ok=True)
    paths_before_copy = [p.relative_to(case_dir) for p in case_dir.rglob("*")]
    pm = PacksManager(case_dir)
    examples_dict = pm.available_examples()
    copy_examples(examples_dict, target_dir=target_dir)
    if not expected:
        paths_after_copy = [p.relative_to(case_dir) for p in case_dir.rglob("*")]
        assert paths_after_copy == paths_before_copy
    else:
        dest_root = target_dir or case_dir
        for rel_path in expected:
            dest_file = dest_root / rel_path
            assert dest_file.exists(), f"{dest_file} missing"

sbillinge

Please see content

cadenmyers13 · 2025-10-14T14:33:06Z

src/diffpy/cmi/cli.py

    examples_dict : dict
        Dictionary mapping pack name -> list of (example, path) tuples.
+    user_input : list of str
+        List of user-specified pack(s) or example(s) to copy.


please review. Think this would be useful for parsing examples_dict

cadenmyers13 · 2025-10-14T14:35:09Z

@sbillinge I changed the copy_examples() API to accept user_input to help with parsing examples (see inline comment). Let me know if you think this works.

cadenmyers13 · 2025-10-14T16:03:38Z

@sbillinge Ready for review. I updated the test such that it takes the actual cli command

sbillinge · 2025-10-14T16:04:27Z

for the tests I like the new test, it is cleaner, but my suggestion would be to:

in our tmpfile infrastructure create files called cwd and user_target. do this in conftest.py
when we start the test do os.chdir('cwd') or sthg like that, so the actual cwd is always tmpdir / "cwd"
In the pytest.parametrize hvae inputs as (<case>, <target>), where <target> is either None or "user_target".
You can test the copied files are in the directory that is relative to the current directory, because cwd is always tmpdir / "cwd" and you can figure out the relative path from there.

tests/test_cli.py

cadenmyers13 · 2025-10-14T17:45:22Z

@sbillinge ready for review. I pasted the new tmp dir structure here #54 (comment)

Note, still need to update behavior comments, but will do that after lunch!

cadenmyers13 · 2025-10-14T19:33:52Z

tests/test_cli.py

+                assert dest_file.exists()
+    else:
+        empty_dir = list(target_dir.rglob("*"))
+        assert not empty_dir, f"Expected nothing, but found: {empty_dir}"


Because we have expected paths that are empty (ie case1 and case4), these cases pass even though copy_examples isnt built. This will fail if copy_examples copies anything into the target directory.

tests/test_cli.py::test_copy_examples[case1-user_inputs0-expected0-None] PASSED [ 10%] tests/test_cli.py::test_copy_examples[case1-user_inputs0-expected0-user_target] PASSED [ 20%] tests/test_cli.py::test_copy_examples[case2-user_inputs1-expected1-None] FAILED [ 30%] tests/test_cli.py::test_copy_examples[case2-user_inputs1-expected1-user_target] FAILED [ 40%] tests/test_cli.py::test_copy_examples[case3-user_inputs2-expected2-None] FAILED [ 50%] tests/test_cli.py::test_copy_examples[case3-user_inputs2-expected2-user_target] FAILED [ 60%] tests/test_cli.py::test_copy_examples[case4-user_inputs3-expected3-None] PASSED [ 70%] tests/test_cli.py::test_copy_examples[case4-user_inputs3-expected3-user_target] PASSED [ 80%] tests/test_cli.py::test_copy_examples[case5-user_inputs4-expected4-None] FAILED [ 90%] tests/test_cli.py::test_copy_examples[case5-user_inputs4-expected4-user_target] FAILED [100%]

tests/test_cli.py

sbillinge

the cases are all good now, but this testing is all in the wrong direction to my taste. Very convoluted and not simple clear actual == expected things. I recommend to start over and make it just follow the pattern that in the paramaterize we pass in something like [(input1, expected1)] and the test doesn't have any, or a tiny amount, of logic it just looks something like

def test_function(inputs, expected)
    actual = function(inputs)
    assert actual == expected

sbillinge

please see comments

tests/test_cli.py

cadenmyers13 · 2025-10-15T18:58:46Z

@sbillinge Alright, ready for review!

sbillinge

this is very close now. Just a few cleaning edits and we are done.

src/diffpy/cmi/cli.py

src/diffpy/cmi/packsmanager.py

tests/test_packsmanager.py

cadenmyers13 · 2025-10-16T14:49:49Z

@sbillinge ready for review.

sbillinge

please see comments

sbillinge · 2025-10-17T09:56:17Z

tests/test_packsmanager.py

+    examples_dir = example_cases / "case5"
+    pm = PacksManager(root_path=examples_dir)
+    target_dir = example_cases / "user_target"
+    actual = pm.copy_examples(examples_to_copy=input, target_dir=target_dir)


I see two possible issues here that are issues with the function signature.

it looks as if examples_to_copy is an optional parameter but maybe it should be required. If it remains optional we need to decide what the default behavior is. I guess, arguably, the default behavior could be to copy all the examples. So the decision is (a) make it required, not optional. or (b) leave it optional but have the default behavior to copy everything.

You seem to assigned what this function returns to be actual, but this is not testing the behavior. The test should be that the files are actually copied correctly, not something that the function returns. In fact, arguably, the function should return None.

For 1, It isn't an optional parameter. it should be require. I think the confusion comes from me being explicit about the variable name (ie examples_to_copy=input rather than input). I'll remove the param name here (and everywhere else that i may have left it).

For 2, I'll work on this right now. I'll get the path to the copied examples separately. Then, copy_examples will return None.

sbillinge · 2025-10-17T10:06:00Z

tests/test_packsmanager.py

+    actual = pm.copy_examples(examples_to_copy=input, target_dir=target_dir)
+    expected = []
+    for path in expected_paths:
+        root_path = target_dir / path


root path is a bad name here. In what sense is this a root_path?

How about this for this work:

expected = [target_dir / path for path in expected_paths]

Then to test the files were copied correctly we would need something like

actual_gen = target_dir.rglob('*')

and syntax that iterates through the lists and (a) checks that they contain the same elements and (b) that the files exist and (c) optionally, but not necessarily, check that the files contain the right contents. I think (c) may be overkill, but strictly if we don't we could write the function that passes the test just by touching those files and not copying them. It will be some work though because I think we are creating those files empty so we would have to write something in them. I think there is a way to create and write them in one line though in conftest, so it could be quick to do. Let's just think about it.

tests/test_packsmanager.py

sbillinge · 2025-10-17T10:08:51Z

tests/test_packsmanager.py

+    examples_dir = example_cases / "case5"
+    os.chdir(example_cases / "cwd")
+    pm = PacksManager(root_path=examples_dir)
+    paths = pm.copy_examples(examples_to_copy=["packA"], target_dir=input)


the comments above are relevant here.

sbillinge · 2025-10-17T10:09:46Z

tests/test_packsmanager.py

+    [
+        (  # input not found (example or pack)
+            ["bad_example"],
+            ValueError,


normally we would check the error message. In this case we use the tests to define good (helpful) error messages.

sbillinge · 2025-10-17T10:17:46Z

tests/test_packsmanager.py

+            ValueError,
+            None,
+        ),
+        (  # path to dir already exists


please use the pattern # description of case. Expected behavior In general, this pattern is better above too, though the behavior is simple and captured well in the test, but here it deserves discussion. What do we want the function to do in this situation?

From a user perspective, I probably don't want a crash. For example, let's say I copy packA to begin with, then I love the code so much that I want to just copy all later, but I want the examples nice and tidily in the same location. When I type all it will find the already present packA examples. Rather than a crash here, I would probably want it to either skip over the already existing examples or overwrite them. To accomplish this, we could interrupt the program and ask user input, but much better is probably to have a default behavior that skips directories that exist and sends a warning, but also to have an option in the cli that is --force or --overwrite, that does the other behavior. The warning that gets printed could mention to rerun the program using --force if you want to overwrite existing examples. SOmething like that.

Yes, I like the --force option, added it here.

sbillinge · 2025-10-17T10:18:36Z

tests/test_packsmanager.py

+@pytest.mark.parametrize(
+    "bad_inputs,expected",
+    [
+        (Path("nonexistent/path/target"), FileNotFoundError),  # doesn't exist


please test error messages here too.

tests/conftest.py

… same content

cadenmyers13 · 2025-10-17T14:55:09Z

@sbillinge ready for review. I also left one response here #60 (comment).

I used the following syntax to capture error messages, let me know if there is a better way to capture this

    with pytest.raises(Exception, match=re.escape(expected)):

Also, before you merge, let me know if it looks good and i can put this all on a clean PR

sbillinge

please see comments

tests/test_packsmanager.py

sbillinge · 2025-10-17T21:20:32Z

tests/test_packsmanager.py

+    expected = [target_dir / path for path in expected_paths]
+    assert actual == expected
+    for path in expected_paths:
+        dest = target_dir / path


this probably easier to read if we use the actual expected language rather than source and dest as we are not moving or copying anything. This looks like code that chatGPT would write, but please try and think like a reviewer if/when adopting such code.....

sbillinge · 2025-10-17T21:24:29Z

tests/test_packsmanager.py

+            # 4) Path to directory already exists.
+            # Expected: Raise a warning with the message.
+            ["ex1"],
+            "Target directory already exists. To overwrite use --force.",


put the behavior explicitly, something like ".... . Current versions of existing files have been left. To overwrite with new versions, please rerun specifying --force" Also, is it target directory or `"example directory: ex1" '

What behavior/message do we want if multiple examples are bad? We don't have test cases for that I think.

I think print the list instead dirs will be okay, something like Target directory(ies) <list> already exist...

sbillinge · 2025-10-17T21:25:12Z

tests/test_packsmanager.py

+            Path("docs/examples/"),
+        ),
+        (
+            # 5) No input provided.


this one is not needed. IT will be handled in the cli not here

sbillinge · 2025-10-17T21:28:05Z

tests/test_packsmanager.py

+        (
+            # 1) Target doesn't exist.
+            # Expected: Raise an error that the target directory is missing.
+            Path("nonexistent/path/target"),


if the path doesn't exist, don't we want to create it?

yes we can create it

sbillinge · 2025-10-17T21:38:57Z

tests/test_packsmanager.py

+            "Target directory does not exist",
+        ),
+        (
+            # 2) Target is a file.


I don't think we need this one. We don't have to worry about this case. I guess the code will crash if we try and create a directory that is a file but we don't need a friendly error message that we test.

sbillinge · 2025-10-17T21:39:29Z

tests/test_packsmanager.py

+            "Target path is not a directory",
+        ),
+        (
+            # 3) Target is nested in a file.


see above. TL:DR, I think we can remove this whole paramaterize

okay, will remove

cadenmyers13 · 2025-10-17T22:06:44Z

@sbillinge ready for review

sbillinge

OK, 100% good on the tests now. Do you want to make a new PR with this version and then punch out the function?

cadenmyers13 · 2025-10-18T17:57:42Z

@sbillinge yep let's do that! I can probably get to it later today. On my phone right now and not home

cadenmyers13 · 2025-10-20T19:54:59Z

closing bc opened a clean PR

cadenmyers13 added 3 commits October 13, 2025 11:56

test for copy_examples

eb85d97

news

918c559

rm comment

6a26c9e

cadenmyers13 commented Oct 13, 2025

View reviewed changes

tests/test_cli.py Outdated Show resolved Hide resolved

sbillinge reviewed Oct 13, 2025

View reviewed changes

tests/test_cli.py Outdated Show resolved Hide resolved

tests/test_cli.py Outdated Show resolved Hide resolved

rm old tests

3ab2efb

sbillinge reviewed Oct 14, 2025

View reviewed changes

add user_input as param for copy_examples

678d0b9

cadenmyers13 commented Oct 14, 2025

View reviewed changes

add user inputs to test

15fd82a

cadenmyers13 commented Oct 14, 2025

View reviewed changes

tests/test_cli.py Outdated Show resolved Hide resolved

cadenmyers13 added 2 commits October 14, 2025 13:42

add cwd and user_target at same level as cases in tmp dir

d57e7fa

modify test for new tmp dir structure

79fa679

fix testing matrix

a1fcf3b

cadenmyers13 commented Oct 14, 2025

View reviewed changes

cadenmyers13 added 4 commits October 14, 2025 16:52

add test for bad copy cases

fc1d0b0

rm docstring

a50c442

Add FileExistError test case

a04de5a

reverse logic for target_dir

aa453b7

cadenmyers13 commented Oct 14, 2025

View reviewed changes

tests/test_cli.py Outdated Show resolved Hide resolved

sbillinge reviewed Oct 14, 2025

View reviewed changes

sbillinge reviewed Oct 15, 2025

View reviewed changes

tests/test_cli.py Outdated Show resolved Hide resolved

tests/test_cli.py Outdated Show resolved Hide resolved

cadenmyers13 added 2 commits October 15, 2025 14:44

add additional dirs to tmp dir

e75a339

move copy_examples inside PacksManager

f750453

cadenmyers13 added 3 commits October 15, 2025 14:48

move copy_examples tests inside pm test

49e745e

rm tests from test_cli

14fac59

add bad_target test

909da70

sbillinge reviewed Oct 16, 2025

View reviewed changes

cadenmyers13 added 3 commits October 16, 2025 10:47

rm duplicate copy_examples() in cli.py

e66eb5f

update docstring and param name

42553c1

wrap str as list, add 4) to bad input test

ef792d2

sbillinge reviewed Oct 17, 2025

View reviewed changes

cadenmyers13 added 5 commits October 17, 2025 09:44

rm param name from func signature

4dc1538

change scope from session to function

5820dd3

rglob examples after they are copied

84e54a8

write text to the files and make check that the copied files have the…

6c38885

… same content

check exception message rather than the exception type

3405e17

copy_examples returns None

b8e4336

sbillinge reviewed Oct 17, 2025

View reviewed changes

sort list of paths, rm bad target test

1324185

sbillinge reviewed Oct 18, 2025

View reviewed changes

cadenmyers13 mentioned this pull request Oct 20, 2025

test: copy_examples test #65

Merged

cadenmyers13 closed this Oct 20, 2025

cadenmyers13 deleted the copy-func2 branch November 3, 2025 14:47

Test: test for copy_examples #60

Test: test for copy_examples #60

Uh oh!

Conversation

cadenmyers13 commented Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cadenmyers13 commented Oct 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

sbillinge left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cadenmyers13 commented Oct 13, 2025

Uh oh!

sbillinge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cadenmyers13 commented Oct 14, 2025

Uh oh!

cadenmyers13 commented Oct 14, 2025

Uh oh!

sbillinge commented Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

cadenmyers13 commented Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sbillinge left a comment

Choose a reason for hiding this comment

Uh oh!

sbillinge left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

cadenmyers13 commented Oct 15, 2025

Uh oh!

sbillinge left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cadenmyers13 commented Oct 16, 2025

Uh oh!

sbillinge 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!

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!

Test: test for `copy_examples` #60

Test: test for `copy_examples` #60

cadenmyers13 commented Oct 13, 2025 •

edited

Loading

cadenmyers13 commented Oct 13, 2025 •

edited

Loading

sbillinge commented Oct 14, 2025 •

edited

Loading

cadenmyers13 commented Oct 14, 2025 •

edited

Loading

cadenmyers13 commented Oct 17, 2025 •

edited

Loading