Enhance tests for 'sh' and 'bat' shells #713
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #713 +/- ##
=======================================
Coverage 86.98% 86.98%
=======================================
Files 69 69
Lines 4088 4088
Branches 706 706
=======================================
Hits 3556 3556
Misses 421 421
Partials 111 111 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
cottsay
force-pushed
the
cottsay/shell-tests
branch
from
d4b31a7 to
cba4e28
Compare
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.
cottsay
force-pushed
the
cottsay/shell-tests
branch
from
cba4e28 to
e31f37f
Compare
There was a problem hiding this comment.
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 😄
| extension.create_prefix_script(prefix_path, False) | ||
| assert (prefix_path / 'local_setup.bat').exists() | ||
|
|
||
| # create_hook_append_value |
There was a problem hiding this comment.
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.
| for name in env_state: | ||
| # skip variables that already had values before this script started prepending | ||
| if name in os.environ: | ||
| if os.environ.get(name): |
There was a problem hiding this comment.
If the intention that this script should only continue if there is any content in the pathname then yes this is a fix for that
| # create_hook_prepend_value | ||
| prepend_hook_path = extension.create_hook_prepend_value( | ||
| 'prepend_env_hook_name', prefix_path, 'pkg_name', | ||
| 'PREPEND_NAME', 'subdirectory') |
There was a problem hiding this comment.
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.
| extension.create_package_script( | ||
| prefix_path, 'pkg_name', [ | ||
| ('hookA.bat', '/some/path/hookA.bat'), | ||
| (append_hook_path.relative_to(prefix_path), ()), |
There was a problem hiding this comment.
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.
| subdirectory_path = str(prefix_path / 'subdirectory') | ||
|
|
||
| # validate appending/prepending without existing values | ||
| with patch.dict(os.environ) as env_patch: |
There was a problem hiding this comment.
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.
| # create_prefix_script | ||
| extension.create_prefix_script(prefix_path, True) | ||
| prefix_script = prefix_path / 'local_setup.bat' | ||
| prefix_script.exists() |
There was a problem hiding this comment.
Wait... This might be mistake. Shouldn't prefix_script.exists() be an assert as well?
There was a problem hiding this comment.
| prefix_script.exists() | |
| assert prefix_script.exists() |
| if sys.platform == 'win32': | ||
| subdirectory_path = str(prefix_path / 'subdirectory') | ||
|
|
||
| # validate appending/prepending without existing values |
There was a problem hiding this comment.
Same test as the one int _test_extensions, but now with local_setup.bat. As are the 'existing values' and 'unique values' tests.
| subdirectory_path, | ||
| 'control', | ||
| )) | ||
| # TODO: The DsvShell behavior doesn't align with BatShell! |
There was a problem hiding this comment.
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?
| # create_prefix_script | ||
| extension.create_prefix_script(prefix_path, True) | ||
| prefix_script = prefix_path / 'local_setup.sh' | ||
| prefix_script.exists() |
There was a problem hiding this comment.
Same here as in the test_shell_bat_.py: Shouldn't this be an assert?
There was a problem hiding this comment.
| prefix_script.exists() | |
| assert prefix_script.exists() |
| )) | ||
|
|
||
|
|
||
| async def _run_prefix_script(prefix_script): |
There was a problem hiding this comment.
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?
|
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. |
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.