Skip to content

Conversation

@leander-dsouza
Copy link
Contributor

@leander-dsouza leander-dsouza commented Oct 10, 2025

Basic Info

Info Result
Primary OS tested on Ubuntu

Description of contribution in a few bullet points

  • This PR adds base integration tests for colcon mixin subverbs, covering the following:

    • Adding mixin repositories
    • Updating mixin repositories
    • Showing available mixins
    • Removing mixin repositories
    • Error handling for invalid inputs and duplicate operations
  • Included a couple of mixin files - build-type.mixin and coverage.mixin from the colcon-mixin-repository to serve as a basis for all repository testing.

Description of how this change was tested

  • Ensured all the tests pass locally - python3 -m pytest test.

Signed-off-by: Leander Stephen D'Souza <[email protected]>
@codecov-commenter
Copy link

codecov-commenter commented Oct 10, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 45.90%. Comparing base (ebedd27) to head (0015daa).

Additional details and impacted files
@@             Coverage Diff             @@
##           master      #54       +/-   ##
===========================================
+ Coverage   15.30%   45.90%   +30.60%     
===========================================
  Files          11       11               
  Lines         562      562               
  Branches       94       94               
===========================================
+ Hits           86      258      +172     
+ Misses        474      282      -192     
- Partials        2       22       +20     

☔ 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.

@leander-dsouza leander-dsouza force-pushed the test-colcon-mixin-add branch 3 times, most recently from 2bc9c68 to 13390bc Compare October 10, 2025 11:32
@leander-dsouza leander-dsouza marked this pull request as ready for review October 10, 2025 11:40
@cottsay
Copy link
Member

cottsay commented Oct 14, 2025

Thanks for making some progress on the tests.

Invoking the entirety of colcon as part of the tests here isn't something we'll want to do. There are a variety of reasons, but to name a couple:

  1. The developer may have any number of extensions enabled, some of which may interfere with the test expectations.
  2. The in-test invocations could cause other extensions to act unpredictably, such as colcon-rerun, which records invocations.

Other extensions test their verbs without using subprocess invocations, and we should do the same here. Examples:

Signed-off-by: Leander Stephen D'Souza <[email protected]>
@leander-dsouza
Copy link
Contributor Author

Invoking the entirety of colcon as part of the tests here isn't something we'll want to do.

Thank you for attaching the documentation of the testing pattern of other members of colcon.
I have updated the tests to use CommandContext instead of subprocess to validate each subverb usage.

@cottsay cottsay added this to the 0.2.4 milestone Oct 20, 2025
@cottsay cottsay added the enhancement New feature or request label Oct 20, 2025
@cottsay cottsay self-assigned this Oct 20, 2025
Copy link
Member

@cottsay cottsay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, this is looking a lot better. Looking forward to getting it merged and seeing that coverage number go up.

Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

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 show subverb 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.txt to have an expected YAML output.

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

@leander-dsouza leander-dsouza Oct 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have modified the mixins to use a YAML format instead.
However, yamllint cannot check for linting in these files, as these have to end in a .mixin extension for discovery.

Signed-off-by: Leander Stephen D'Souza <[email protected]>
This is no longer needed after 9f2dc67
was merged.

Additionally, use a pytest temporary directory for staging the config
files rather than a static location.
@cottsay
Copy link
Member

cottsay commented Nov 25, 2025

I merged master and added a commit to remove the special casing around get_config_path now that #55 is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Development

Successfully merging this pull request may close these issues.

3 participants