Skip to content
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

Rework Package identification tests #229

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

rotu
Copy link
Contributor

@rotu rotu commented Sep 13, 2019

Package identifications test was doing way too much in one test to follow.
Split into multiple, more targeted tests and added some additional coverage

@rotu rotu force-pushed the better_package_identification_tests branch from f69a854 to 5d17a59 Compare September 13, 2019 18:46
@dirk-thomas dirk-thomas added the in progress Actively being worked on (Kanban column) label Sep 13, 2019
@codecov-io
Copy link

codecov-io commented Sep 13, 2019

Codecov Report

Merging #229 into master will decrease coverage by 0.18%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #229      +/-   ##
==========================================
- Coverage   79.50%   79.32%   -0.19%     
==========================================
  Files          55       55              
  Lines        3221     3221              
  Branches      535      535              
==========================================
- Hits         2561     2555       -6     
- Misses        613      617       +4     
- Partials       47       49       +2     
Impacted Files Coverage Δ
colcon_core/package_identification/python.py 78.16% <0.00%> (-6.90%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 165c033...04713ee. Read the comment docs.

@dirk-thomas
Copy link
Member

In the current state the patch reduced test coverage in some part of the code.

@dirk-thomas
Copy link
Member

@rotu Friendly ping.

@rotu
Copy link
Contributor Author

rotu commented Sep 19, 2019

Thanks for the ping.
The coverage dip is due to the xfail on test_re_identify_python_if_different_python_package. That's the downside of breaking this up into two PRs.

@rotu rotu force-pushed the better_package_identification_tests branch from 5d9c766 to a9f8179 Compare September 19, 2019 22:36
@rotu
Copy link
Contributor Author

rotu commented Sep 25, 2019

@dirk-thomas Friendly ping.

@dirk-thomas dirk-thomas added review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Sep 26, 2019
Copy link
Member

@dirk-thomas dirk-thomas left a comment

Choose a reason for hiding this comment

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

Thanks for improving the tests.

I will just wait for clicking merge until #215 is good to be merged (to avoid the temporary drop of coverage between them).

@dirk-thomas
Copy link
Member

Please rebase / resolve conflict to be able to re-review and merge this patch.

@dirk-thomas dirk-thomas added in progress Actively being worked on (Kanban column) and removed review Waiting for review (Kanban column) labels Mar 27, 2020
@dirk-thomas
Copy link
Member

@rotu Please rebase this branch in a way that the pull requests only shows commits related to this change.

@rotu rotu force-pushed the better_package_identification_tests branch from 8c4e3b3 to 4912050 Compare March 27, 2020 20:14
@rotu
Copy link
Contributor Author

rotu commented Mar 27, 2020

@rotu Please rebase this branch in a way that the pull requests only shows commits related to this change.

Oops. Nothing like rebasing on the wrong master. Blech. Let me try one more time...

Success!

@rotu rotu force-pushed the better_package_identification_tests branch from 4912050 to 04713ee Compare March 27, 2020 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in progress Actively being worked on (Kanban column)
Development

Successfully merging this pull request may close these issues.

None yet

3 participants