Enhance tests for 'sh' and 'bat' shells #713

cottsay · 2025-10-21T18:45:58Z

The existing tests exercise much of the code for implementing colcon's shell subsystem, but it doesn't validate the results. If the platform supports the shell, we should check that the code for appending and prepending paths to lists function as intended.

In each test module, the first block of changes is just a refactor so that we reference the hooks generated by the calls to create_hook_*. Previously, the test only validated that the hook could be generated, and didn't attempt to actually use it.

The second block adds a handful of assertions about the behavior of the hooks when executed.

codecov · 2025-10-21T18:47:15Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.18%. Comparing base (35e65f6) to head (0a3aa2c).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #713      +/-   ##
==========================================
+ Coverage   86.98%   87.18%   +0.19%     
==========================================
  Files          69       69              
  Lines        4088     4088              
  Branches      706      706              
==========================================
+ Hits         3556     3564       +8     
+ Misses        421      413       -8     
  Partials      111      111

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

The existing tests exercise much of the code for implementing colcon's shell subsystem, but it doesn't validate the results. If the platform supports the shell, we should check that the code for appending and prepending paths to lists function as intended.

knmcguire

Hi! I've tried to dig through this PR. Most of these comments are just more comments if I understand what is actually happening. So feel free to comment on those or just give a thumbs up to those that I don't say anything completely off what is actually happening.

I've focused mostly on the .bat scripts (as that is what I'll mostly look at as committer for windows efforts)
The tests of this PR are failing as it is actually now seeing trailing colons (on macOS), which you already indicated in #712 . So that PR indeed would need to be merged first.
I actually found 2 potentially missed asserts (prefix_script.exists() ) so if that is indeed a mistake then I actually contributed something to this PR 😄

knmcguire · 2026-01-03T13:42:54Z

test/test_shell_bat.py

    extension.create_prefix_script(prefix_path, False)
    assert (prefix_path / 'local_setup.bat').exists()

+    # create_hook_append_value


As I understood it, these are tests for create_hook_append_value() and create_hook_prepend_value() that were already included, but the only thing that changed was that now the hook_path is changed to 2 separate values to be used later.

Yes. The original test only exercised the code that generates the hooks. In order to actually invoke the hooks to check that they do the right thing, I had to change the call site a little bit.

colcon_core/shell/template/prefix_util.py.em

knmcguire · 2026-01-03T13:45:47Z

test/test_shell_bat.py

+    # create_hook_prepend_value
+    prepend_hook_path = extension.create_hook_prepend_value(
+        'prepend_env_hook_name', prefix_path, 'pkg_name',
+        'PREPEND_NAME', 'subdirectory')


To rename "NAME" and "env_hook_name.bat" would also be a better cleanup indeed.

Just a thought... would it make sense to also have a different name for the subdirectory? like 'append_subdirectory' or 'prepend_subdirectory' ? Probably won't matter in practice but it might make things clearer at the later tests.

To rename "NAME" and "env_hook_name.bat" would also be a better cleanup indeed.

👍

Just a thought... would it make sense to also have a different name for the subdirectory? like 'append_subdirectory' or 'prepend_subdirectory' ? Probably won't matter in practice but it might make things clearer at the later tests.

I think I'll decline that change for now. It would validate that we never "cross the streams" between the hooks, but I don't think there's a strong risk of that happening. The added complexity to this (already complicated) test probably doesn't justify the additional coverage.

knmcguire · 2026-01-03T13:47:26Z

test/test_shell_bat.py

    extension.create_package_script(
        prefix_path, 'pkg_name', [
-            ('hookA.bat', '/some/path/hookA.bat'),
+            (append_hook_path.relative_to(prefix_path), ()),


Good efficient use of the earlier made temporary hookfiles to use for this test, which also tests if no 'non bat files' are added to the path as well.

knmcguire · 2026-01-03T14:05:53Z

test/test_shell_bat.py

+        subdirectory_path = str(prefix_path / 'subdirectory')
+
+        # validate appending/prepending without existing values
+        with patch.dict(os.environ) as env_patch:


So these tests checks if nothing weird is added both the APPEND_NAME and PREPEND_NAME path (where the hookscripts are located) if only none is added (then it should be just the initial path names). I haven't worked with mock.patch myself but I looked it up and learned these changes to the environment paths here in the with will not actually happen in the real environment path (hence it comes from mock). So I hope I've understood this correctly.

So these tests checks if nothing weird is added both the APPEND_NAME and PREPEND_NAME path...

Not quite. I'm using dict.pop to remove the values from os.environ (or rather, the mock of os.environ). I'm not 100% certain, but I think this is just a safeguard just in case someone invokes the test with those environment variables set to something already. There doesn't seem to be a good way to use patch.dict to remove items in a single call, so I had to make follow-up calls to "set up" the mock for the actual test.

I haven't worked with mock.patch myself but I looked it up and learned these changes to the environment paths here in the with will not actually happen in the real environment path (hence it comes from mock).

Yeah, we're essentially replacing the os.environ Python object with a shim. In this case, the mocked os.environ is the same as the original, but with APPEND_NAME and PREPEND_NAME removed (if they were present at all). When the mock scope exits, the original os.environ object is restored.

I've encountered issues with Python's handling of environment variables in other contexts, if you're interested. In short, the os.environ dict is really just a cache of the process' actual environment table, and it's possible for them to get out of sync.

knmcguire · 2026-01-03T14:46:47Z

test/test_shell_bat.py

+    # create_prefix_script
+    extension.create_prefix_script(prefix_path, True)
+    prefix_script = prefix_path / 'local_setup.bat'
+    prefix_script.exists()


Wait... This might be mistake. Shouldn't prefix_script.exists() be an assert as well?

Suggested change

prefix_script.exists()

assert prefix_script.exists()

Well spotted! I'll update.

knmcguire · 2026-01-03T14:47:48Z

test/test_shell_bat.py

+    if sys.platform == 'win32':
+        subdirectory_path = str(prefix_path / 'subdirectory')
+
+        # validate appending/prepending without existing values


Same test as the one int _test_extensions, but now with local_setup.bat. As are the 'existing values' and 'unique values' tests.

👍 Same assertions, different process.

knmcguire · 2026-01-03T14:50:17Z

test/test_shell_bat.py

+                subdirectory_path,
+                'control',
+            ))
+            # TODO: The DsvShell behavior doesn't align with BatShell!


Ah oke! Does that mean that this test will assert due to this behavior?

I know too little about dsv-shells... but will this assert always fail even though the behavior is correct?

This test is asserting the existing behavior. If DsvShell is what we decide to change, then this test will need to be updated when the behavior is reconciled.

I'd guess we could no-op or comment out the check until that time, but even given the differing behavior the test is still useful to prevent regression.

knmcguire · 2026-01-03T15:02:45Z

test/test_shell_sh.py

+    # create_prefix_script
+    extension.create_prefix_script(prefix_path, True)
+    prefix_script = prefix_path / 'local_setup.sh'
+    prefix_script.exists()


Same here as in the test_shell_bat_.py: Shouldn't this be an assert?

Suggested change

prefix_script.exists()

assert prefix_script.exists()

knmcguire · 2026-01-03T15:03:43Z

test/test_shell_sh.py

+            ))
+
+
+async def _run_prefix_script(prefix_script):


Ah oke, Didn't know that this was a difference between sh and bat, that in command prompt (bat) the seperation of environment variables worked differently.

Wouldn't this try except work for the .bat test script as well? and if so, then could these test_files for both bat and sh files also result from a template file?

The only reason this try/except is here is to deal with implementations of env which don't support null separation. The null separation is useful for dealing with environment variable values which contain newlines, so we prefer that method when supported.

For bat, we're using set and not env. I don't believe set can output with null separation. On that note, it would be a good idea to validate that we're handling newlines properly in bat at some point. Maybe there's a similar concept we can leverage but I haven't actually looked.

knmcguire · 2026-01-03T15:41:53Z

Also another question, perhaps more of a design question than about this PR... how about the local_setup.ps1 scripts for powershell? those are generated but not tested I see.

cottsay · 2026-01-16T23:11:04Z

Also another question, perhaps more of a design question than about this PR... how about the local_setup.ps1 scripts for powershell? those are generated but not tested I see.

Shells other than sh and bat are part of separate extensions, such as this one: https://github.com/colcon/colcon-powershell

They should have their own tests. It's very likely that the additional checks I'm adding here are appropriate for those other shells as well, but I'm not sure I have the bandwidth to port them over any time soon. Help Wanted 😁

Co-authored-by: knmcguire <kimberleymcguire@gmail.com>

cottsay

This was a really helpful and thorough review, @knmcguire. Thank you.

Sorry it took so long to get back to you.

colcon_core/shell/template/prefix_util.py.em

cottsay · 2026-01-28T17:55:17Z

test/test_shell_bat.py

+    # create_hook_prepend_value
+    prepend_hook_path = extension.create_hook_prepend_value(
+        'prepend_env_hook_name', prefix_path, 'pkg_name',
+        'PREPEND_NAME', 'subdirectory')


To rename "NAME" and "env_hook_name.bat" would also be a better cleanup indeed.

👍

Just a thought... would it make sense to also have a different name for the subdirectory? like 'append_subdirectory' or 'prepend_subdirectory' ? Probably won't matter in practice but it might make things clearer at the later tests.

I think I'll decline that change for now. It would validate that we never "cross the streams" between the hooks, but I don't think there's a strong risk of that happening. The added complexity to this (already complicated) test probably doesn't justify the additional coverage.

cottsay · 2026-01-28T17:55:43Z

test/test_shell_bat.py

    extension.create_package_script(
        prefix_path, 'pkg_name', [
-            ('hookA.bat', '/some/path/hookA.bat'),
+            (append_hook_path.relative_to(prefix_path), ()),


cottsay · 2026-01-28T18:05:54Z

test/test_shell_bat.py

+        subdirectory_path = str(prefix_path / 'subdirectory')
+
+        # validate appending/prepending without existing values
+        with patch.dict(os.environ) as env_patch:


So these tests checks if nothing weird is added both the APPEND_NAME and PREPEND_NAME path...

Not quite. I'm using dict.pop to remove the values from os.environ (or rather, the mock of os.environ). I'm not 100% certain, but I think this is just a safeguard just in case someone invokes the test with those environment variables set to something already. There doesn't seem to be a good way to use patch.dict to remove items in a single call, so I had to make follow-up calls to "set up" the mock for the actual test.

I haven't worked with mock.patch myself but I looked it up and learned these changes to the environment paths here in the with will not actually happen in the real environment path (hence it comes from mock).

Yeah, we're essentially replacing the os.environ Python object with a shim. In this case, the mocked os.environ is the same as the original, but with APPEND_NAME and PREPEND_NAME removed (if they were present at all). When the mock scope exits, the original os.environ object is restored.

I've encountered issues with Python's handling of environment variables in other contexts, if you're interested. In short, the os.environ dict is really just a cache of the process' actual environment table, and it's possible for them to get out of sync.

cottsay · 2026-01-28T18:06:45Z

test/test_shell_bat.py

+            assert env.get('APPEND_NAME') == subdirectory_path
+            assert env.get('PREPEND_NAME') == subdirectory_path
+
+        # validate appending/prepending with existing values


the append and prepend hook scripts are supposed to add the subdirectory path to the append and prepend paths.

💯

cottsay · 2026-01-28T21:21:45Z

test/test_shell_bat.py

+    dsv_extension = DsvShell()
+
+    # create_hook_append_value
+    append_hook_path = dsv_extension.create_hook_append_value(


Yeah, the two extensions operate in parallel to support two different scenarios. See my other comment on how they differ. In short, it should be possible to transition away from the native hooks and only produce dsv hooks for these well-defined operations like appending/prepending, and it would probably be more performant. Not something I'm currently tracking though.

cottsay · 2026-01-28T21:22:23Z

test/test_shell_bat.py

+    if sys.platform == 'win32':
+        subdirectory_path = str(prefix_path / 'subdirectory')
+
+        # validate appending/prepending without existing values


👍 Same assertions, different process.

cottsay · 2026-01-28T21:24:44Z

test/test_shell_bat.py

+                subdirectory_path,
+                'control',
+            ))
+            # TODO: The DsvShell behavior doesn't align with BatShell!


This test is asserting the existing behavior. If DsvShell is what we decide to change, then this test will need to be updated when the behavior is reconciled.

I'd guess we could no-op or comment out the check until that time, but even given the differing behavior the test is still useful to prevent regression.

cottsay · 2026-01-28T21:31:12Z

test/test_shell_sh.py

+            ))
+
+
+async def _run_prefix_script(prefix_script):


The only reason this try/except is here is to deal with implementations of env which don't support null separation. The null separation is useful for dealing with environment variable values which contain newlines, so we prefer that method when supported.

For bat, we're using set and not env. I don't believe set can output with null separation. On that note, it would be a good idea to validate that we're handling newlines properly in bat at some point. Maybe there's a similar concept we can leverage but I haven't actually looked.

cottsay · 2026-01-28T21:32:40Z

test/test_shell_bat.py

    extension.create_prefix_script(prefix_path, False)
    assert (prefix_path / 'local_setup.bat').exists()

+    # create_hook_append_value


Yes. The original test only exercised the code that generates the hooks. In order to actually invoke the hooks to check that they do the right thing, I had to change the call site a little bit.

Previously the test was setting the variables to an empty value, which isn't the same on POSIX.

cottsay self-assigned this Oct 21, 2025

cottsay added the enhancement New feature or request label Oct 21, 2025

cottsay added this to the 0.20.2 milestone Oct 21, 2025

This was referenced Oct 21, 2025

Simplify removing leading and trailing separators #712

Merged

Conform to linters in prefix_util.py.em #714

Merged

cottsay force-pushed the cottsay/shell-tests branch from d4b31a7 to cba4e28 Compare October 21, 2025 23:35

cottsay force-pushed the cottsay/shell-tests branch from cba4e28 to e31f37f Compare October 21, 2025 23:46

knmcguire reviewed Jan 3, 2026

View reviewed changes

Merge branch 'master' into cottsay/shell-tests

c09127d

PR feedback: Fix missing asserts and string conversion

4fd8769

Co-authored-by: knmcguire <kimberleymcguire@gmail.com>

cottsay commented Jan 28, 2026

View reviewed changes

Revert an unrelated change

0a2f7f9

cottsay force-pushed the cottsay/shell-tests branch from 4cff594 to 0a2f7f9 Compare January 29, 2026 17:04

Actually remove values with verifying behavior

0a3aa2c

Previously the test was setting the variables to an empty value, which isn't the same on POSIX.

Enhance tests for 'sh' and 'bat' shells #713

Are you sure you want to change the base?

Enhance tests for 'sh' and 'bat' shells #713

Conversation

cottsay commented Oct 21, 2025

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

knmcguire left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

knmcguire commented Jan 3, 2026

Uh oh!

cottsay commented Jan 16, 2026

Uh oh!

cottsay left a comment

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!

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

codecov bot commented Oct 21, 2025 •

edited

Loading

knmcguire left a comment •

edited

Loading