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

better tests for unhappy paths #100

Open
PietroPasotti opened this issue Nov 15, 2024 · 4 comments
Open

better tests for unhappy paths #100

PietroPasotti opened this issue Nov 15, 2024 · 4 comments

Comments

@PietroPasotti
Copy link
Contributor

Bug Description

our coverage of the interface.py module lacks a little bit when it comes to unhappy-path tests.
In particular, the library is expected to gracefully handle situations when the databag doesn't contain data that is formatted in the expected way, but we aren't testing those paths.

To Reproduce

PYTHONPATH=.:src:lib pytest --cov

Environment

main

Relevant log output

src/cosl/__init__.py                              6      0      0      0   100%
src/cosl/coordinated_workers/__init__.py         20      5      2      0    68%   26-28, 31-32
src/cosl/coordinated_workers/coordinator.py     302     71     84     18    72%   149, 290->293, 377, 384-388, 397, 402, 407->406, 409, 414, 424-425, 481-493, 498-514, 522-544, 568-569, 583->587, 590, 593, 604, 619-620, 627-632, 649-655, 664-665, 700-720, 725, 730, 740-747
src/cosl/coordinated_workers/interface.py       295     69     82     15    73%   81-84, 90->92, 268, 272-277, 294-305, 319, 330-331, 335-337, 342-344, 355, 363->362, 367-369, 384, 389, 393-399, 405-408, 417, 427-433, 467, 470, 474->exit, 481-482, 490-504, 510-511, 525, 528->exit, 571, 582-585, 589-592, 599
src/cosl/coordinated_workers/nginx.py            90     16     24      7    76%   63-90, 97->exit, 105, 112-113, 129->exit, 134->exit, 142-143, 176->exit
src/cosl/coordinated_workers/worker.py          386     82    164     32    76%   58, 214-215, 219-220, 241-243, 249-250, 257, 263, 269-272, 284-289, 321, 323, 342, 362-366, 442-445, 459, 461, 465, 475, 481, 518-529, 556-557, 560-561, 574, 595-598, 605->609, 627, 662-663, 665-666, 668, 690-696, 705-707, 729-733, 743, 749, 766-794, 798-805, 827, 836->840, 841->840, 855-857

Additional context

focus on src/cosl/coordinated_workers/interface.py 330-331, 335-337, 342-344,

those are 'exception paths' in the cosl.coordinated_workers.interface.ClusterProvider.gather_addresses_by_role method

we should add one or multiple tests that exercise those blocks of code.

Those tests should live in the tests/test_coordinated_workers/test_coordinator.py module and use pytest and scenario.

Rough explanation of what the method is doing:

the ClusterProvider object is a relation endpoint wrapper for the 'cluster' relation interface.
That piece of code is responsible for walking through all units related over 'cluster' and collect a piece of data from their databags.
The expectation is that a log will be printed out if that fails (e.g. because the databag is badly formatted).

@lorenzo-medici
Copy link
Contributor

I'd like to take on this issue as part of the Engineering Onboarding. Just so that I can update my Jira tickets, should this count as "Bug Fix / Feature" or "Tests" (or both 😃)?

@PietroPasotti
Copy link
Contributor Author

I'd like to take on this issue as part of the Engineering Onboarding. Just so that I can update my Jira tickets, should this count as "Bug Fix / Feature" or "Tests" (or both 😃)?

Welcome to take a shot at it! I think this would qualify in the 'tests' category ;)

@PietroPasotti
Copy link
Contributor Author

@lorenzo-medici do note that a colleague of yours already worked on some of those paths in #102

for some reason the PR wasn't linked to this feature, but so something has been done already. There's more still, you can re-run the coverage tool and see what's missing

@lorenzo-medici
Copy link
Contributor

The lines in interface.py that are specifically referenced have indeed already been included in the test coverage.
I'll expand the test coverage for ClusterRequirer in the same file, unless you suggest other things.

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

No branches or pull requests

2 participants