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

[ONB-730] test: increase coverage by testing dynamic TLS config generation for TCP routes #386

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

Conversation

JoseFMP
Copy link

@JoseFMP JoseFMP commented Jul 23, 2024

This PR add to the unit tests coverage in the function _update_dynamic_config_route, for the cases where 'tcp' routes are passed in the Charm configuration

Issue

The unit tests do not cover the case of dynamic TLS configuration generation for TCP routes.

Solution

In the mock configuration passed to the unit tests, add TCP routes as well as their expected TLS auto-generated configuration.

@JoseFMP JoseFMP requested a review from a team as a code owner July 23, 2024 06:09
@JoseFMP JoseFMP marked this pull request as draft July 23, 2024 06:15
@JoseFMP JoseFMP marked this pull request as ready for review July 23, 2024 06:41
@JoseFMP JoseFMP changed the title test: increase coverage by testing dynamic TLS config generation for TCP routes [ONB-730] test: increase coverage by testing dynamic TLS config generation for TCP routes Jul 23, 2024
@Abuelodelanada
Copy link
Contributor

Hola @JoseFMP !

This is ok. Anyway if you run the unit tests suite in this repo:

tox -e unit

You will see the following report:

unit: commands[1]> coverage report
Name             Stmts   Miss Branch BrPart  Cover   Missing
------------------------------------------------------------
src/charm.py       586    134    241     44    73%   299->exit, 307, 311-313, 325, 329-333, 339, 345, 351-353, 364, 371, 374-379, 383-390, 394-400, 408, 414, 423, 432-442, 455, 457-458, 461->452, 473->470, 489-498, 527, 543->553, 564-570, 674-675, 690, 713-721, 743-744, 767-769, 772-773, 785, 793-797, 802, 807->834, 819, 822, 835-851, 869->874, 878-884, 892-916, 924-925, 929-931, 946->952, 965, 970-976, 983->1002, 999->1002, 1014, 1029, 1045->1056, 1082-1092, 1097, 1127, 1141-1143, 1149, 1164->exit, 1167-1168, 1176-1191, 1201, 1205
src/traefik.py     243     48     72     15    77%   50, 125-147, 158, 166-169, 176, 179, 191, 199, 211-214, 237, 311-326, 396-397, 434, 474-476, 480, 488-490, 503, 515-519, 540-541, 550, 558, 600-602, 663-664, 669-679
src/utils.py        10      0      2      0   100%
------------------------------------------------------------
TOTAL              839    182    315     59    74%

Do you think you can add one more unit test that increases the coverage?

@PietroPasotti
Copy link
Contributor

Hi @JoseFMP I think it's good to add more input combinations to existing parametrized tests. Thanks!
Could you show us how that affected the coverage specifically? A before/after comparison would be good :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants