-
Notifications
You must be signed in to change notification settings - Fork 9
Add basic tests for mixin subverbs #54
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
83be597
e70b8e3
15be422
dc831ba
d09b50f
0015daa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| build: | ||
| - coverage-gcc | ||
| cmake-args: ["-DCMAKE_C_FLAGS='--coverage'", "-DCMAKE_CXX_FLAGS='--coverage'"] | ||
| - coverage-pytest | ||
| ament-cmake-args: ['-DAMENT_CMAKE_PYTEST_WITH_COVERAGE=ON'] | ||
| - debug | ||
| cmake-args: ['-DCMAKE_BUILD_TYPE=Debug'] | ||
| - min-size-rel | ||
| cmake-args: ['-DCMAKE_BUILD_TYPE=MinSizeRel'] | ||
| - rel-with-deb-info | ||
| cmake-args: ['-DCMAKE_BUILD_TYPE=RelWithDebInfo'] | ||
| - release | ||
| cmake-args: ['-DCMAKE_BUILD_TYPE=Release'] | ||
| test: | ||
| - coverage-pytest | ||
| pytest-args: ['--cov-report=term'] | ||
| pytest-with-coverage: True |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| --- | ||
| mixin: | ||
| - mixins/build-type.mixin | ||
| - mixins/coverage.mixin |
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unless there's a reason to use JSON here, I think we should use YAML. For one, it has better line-change-cleanliness. The filename can remain the same or change, whatever you prefer.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have modified the mixins to use a YAML format instead. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| --- | ||
| build: | ||
| debug: | ||
| cmake-args: | ||
| - "-DCMAKE_BUILD_TYPE=Debug" | ||
| min-size-rel: | ||
| cmake-args: | ||
| - "-DCMAKE_BUILD_TYPE=MinSizeRel" | ||
| rel-with-deb-info: | ||
| cmake-args: | ||
| - "-DCMAKE_BUILD_TYPE=RelWithDebInfo" | ||
| release: | ||
| cmake-args: | ||
| - "-DCMAKE_BUILD_TYPE=Release" |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| --- | ||
| build: | ||
| coverage-gcc: | ||
| cmake-args: | ||
| - "-DCMAKE_C_FLAGS='--coverage'" | ||
| - "-DCMAKE_CXX_FLAGS='--coverage'" | ||
| coverage-pytest: | ||
| ament-cmake-args: | ||
| - "-DAMENT_CMAKE_PYTEST_WITH_COVERAGE=ON" | ||
| test: | ||
| coverage-pytest: | ||
| pytest-args: | ||
| - "--cov-report=term" | ||
| pytest-with-coverage: true |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,167 @@ | ||
| # Copyright 2025 Leander Stephen Desouza | ||
| # Licensed under the Apache License, Version 2.0 | ||
|
|
||
| from argparse import ArgumentTypeError | ||
| from pathlib import Path | ||
| from unittest.mock import Mock | ||
|
|
||
| from colcon_core.command import CommandContext | ||
| from colcon_core.location import set_default_config_path | ||
| from colcon_mixin.mixin.repository import set_repositories | ||
| from colcon_mixin.subverb.add import (_non_empty_string_without_pathsep, | ||
| _url_string, AddMixinSubverb) | ||
| from colcon_mixin.subverb.remove import RemoveMixinSubverb | ||
| from colcon_mixin.subverb.show import ShowMixinSubverb | ||
| from colcon_mixin.subverb.update import UpdateMixinSubverb | ||
| import pytest | ||
|
|
||
|
|
||
| MIXIN_IDENTIFIER = 'demo' | ||
| TEST_URL = Path(__file__).parent.joinpath('index.yaml').absolute().as_uri() | ||
|
|
||
|
|
||
| @pytest.fixture(autouse=True, scope='module') | ||
| def colcon_config_path(tmp_path_factory): | ||
| set_default_config_path(path=tmp_path_factory.mktemp('colcon-mixin.')) | ||
|
|
||
|
|
||
| @pytest.fixture(autouse=True) | ||
| def mixin_cleanup(): | ||
| """Cleanup mixin repositories before and after each test.""" | ||
| set_repositories({}) | ||
| yield | ||
| set_repositories({}) | ||
|
|
||
|
|
||
| def test_add_success(): | ||
| """Test successfully adding a mixin repository.""" | ||
| extension = AddMixinSubverb() | ||
| context = CommandContext(command_name='colcon', args=Mock()) | ||
|
|
||
| context.args.name = MIXIN_IDENTIFIER | ||
| context.args.url = TEST_URL | ||
|
|
||
| result = extension.main(context=context) | ||
| assert result is None | ||
|
|
||
|
|
||
| def test_add_repository_duplicate_name(): | ||
| """Test adding a repository with a name that already exists.""" | ||
| extension = AddMixinSubverb() | ||
|
|
||
| context1 = CommandContext(command_name='colcon', args=Mock()) | ||
| context1.args.name = MIXIN_IDENTIFIER | ||
| context1.args.url = TEST_URL | ||
| result1 = extension.main(context=context1) | ||
| assert result1 is None | ||
|
|
||
| # Second add (duplicate) | ||
| context2 = CommandContext(command_name='colcon', args=Mock()) | ||
| context2.args.name = MIXIN_IDENTIFIER | ||
| context2.args.url = TEST_URL | ||
| result2 = extension.main(context=context2) | ||
| assert result2 == ( | ||
| f"A repository with the name '{MIXIN_IDENTIFIER}' already exists" | ||
| ) | ||
|
|
||
|
|
||
| def test_add_repository_invalid_url(): | ||
| """Test adding a repository with an invalid URL format.""" | ||
| with pytest.raises(ArgumentTypeError, match="must contain '://'"): | ||
| _url_string('invalid-url') | ||
|
|
||
|
|
||
| def test_add_repository_invalid_name(): | ||
| """Test adding a repository with an invalid name.""" | ||
| # Forward slash test | ||
| with pytest.raises(ArgumentTypeError, match="must not contain '/'"): | ||
| _non_empty_string_without_pathsep(f'{MIXIN_IDENTIFIER}/invalid') | ||
|
|
||
| # Backslash test | ||
| with pytest.raises(ArgumentTypeError, match="must not contain '\\\\'"): | ||
| _non_empty_string_without_pathsep(f'{MIXIN_IDENTIFIER}\\invalid') | ||
|
|
||
|
|
||
| def test_update_mixin(capsys): | ||
| """Test updating a mixin repository.""" | ||
| add_extension = AddMixinSubverb() | ||
| add_context = CommandContext(command_name='colcon', args=Mock()) | ||
| add_context.args.name = MIXIN_IDENTIFIER | ||
| add_context.args.url = TEST_URL | ||
| add_extension.main(context=add_context) | ||
|
|
||
| update_extension = UpdateMixinSubverb() | ||
| update_context = CommandContext(command_name='colcon', args=Mock()) | ||
| update_context.args.name = MIXIN_IDENTIFIER | ||
|
|
||
| result = update_extension.main(context=update_context) | ||
| captured = capsys.readouterr() | ||
|
|
||
| assert result == 0 | ||
| assert f'fetching {MIXIN_IDENTIFIER}:' in captured.out | ||
| assert 'build-type.mixin' in captured.out | ||
| assert 'coverage.mixin' in captured.out | ||
|
|
||
|
|
||
| def test_show_mixins(capsys): | ||
| """Test showing available mixins against expected files.""" | ||
| add_extension = AddMixinSubverb() | ||
| add_context = CommandContext(command_name='colcon', args=Mock()) | ||
| add_context.args.name = MIXIN_IDENTIFIER | ||
| add_context.args.url = TEST_URL | ||
| add_extension.main(context=add_context) | ||
|
|
||
| update_extension = UpdateMixinSubverb() | ||
| update_context = CommandContext(command_name='colcon', args=Mock()) | ||
| update_context.args.name = MIXIN_IDENTIFIER | ||
| update_extension.main(context=update_context) | ||
|
|
||
| # Clear previous output from add/update | ||
| capsys.readouterr() | ||
|
|
||
| show_extension = ShowMixinSubverb() | ||
| show_extension.add_arguments(parser=Mock()) # Initialize mixins_by_verb | ||
| show_context = CommandContext(command_name='colcon', args=Mock()) | ||
|
|
||
| # Show all verbs and all mixins | ||
| show_context.args.verb = None | ||
| show_context.args.mixin_name = None | ||
|
|
||
| result = show_extension.main(context=show_context) | ||
| captured = capsys.readouterr() | ||
|
|
||
| assert result is None | ||
|
|
||
| expected_demo_file = Path(__file__).parent / 'expected_mixins' / 'demo.txt' | ||
| with expected_demo_file.open('r', encoding='utf-8') as f: | ||
| expected_content = f.read().strip() | ||
|
|
||
| assert captured.out.strip() == expected_content | ||
|
|
||
|
|
||
| def test_remove_repository_success(): | ||
| """Test successfully removing a mixin repository.""" | ||
| add_extension = AddMixinSubverb() | ||
| add_context = CommandContext(command_name='colcon', args=Mock()) | ||
| add_context.args.name = MIXIN_IDENTIFIER | ||
| add_context.args.url = TEST_URL | ||
| add_extension.main(context=add_context) | ||
|
|
||
| remove_extension = RemoveMixinSubverb() | ||
| remove_context = CommandContext(command_name='colcon', args=Mock()) | ||
| remove_context.args.name = MIXIN_IDENTIFIER | ||
|
|
||
| result = remove_extension.main(context=remove_context) | ||
| assert result is None | ||
|
|
||
|
|
||
| def test_remove_repository_not_exists(): | ||
| """Test removing a repository that doesn't exist.""" | ||
| extension = RemoveMixinSubverb() | ||
| context = CommandContext(command_name='colcon', args=Mock()) | ||
| context.args.name = MIXIN_IDENTIFIER | ||
|
|
||
| result = extension.main(context=context) | ||
| assert result == ( | ||
| f"A repository with the name '{MIXIN_IDENTIFIER}' doesn't exist" | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should align the YAML style with the rest of the project. Mind taking a look at #56, which adds a check for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The show module of colcon-mixin does not print the mixins in a standardised YAML or JSON format. Hence, I cannot change this to an expected YAML output.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoa, bummer. That might be worth filing a new issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have created #58 to ensure that the
showsubverb would print YAML output.If you don't mind, could you please take a look at it?
Upon merging the said PR, I will modify
demo.txtto have an expected YAML output.