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

Resolve some nits in the codebase #708

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

Resolve some nits in the codebase #708

wants to merge 11 commits into from

Conversation

DiamondJoseph
Copy link
Contributor

No description provided.

Copy link

codecov bot commented Nov 11, 2024

Codecov Report

Attention: Patch coverage is 97.14286% with 1 line in your changes missing coverage. Please review.

Project coverage is 92.17%. Comparing base (86c5905) to head (b50fb79).

Files with missing lines Patch % Lines
src/blueapi/__main__.py 0.00% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main     #708   +/-   ##
=======================================
  Coverage   92.17%   92.17%           
=======================================
  Files          35       35           
  Lines        1803     1803           
=======================================
  Hits         1662     1662           
  Misses        141      141           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.


🚨 Try these New Features:

@DiamondJoseph
Copy link
Contributor Author

(some of these type: ignores may need to be reinstated for #619

Copy link
Contributor

@callumforrester callumforrester left a comment

Choose a reason for hiding this comment

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

I like this! Just a few small comments.

src/blueapi/cli/cli.py Show resolved Hide resolved
src/blueapi/core/context.py Show resolved Hide resolved
src/blueapi/worker/task_worker.py Show resolved Hide resolved
@@ -18,7 +22,7 @@ def events(mock_stomp_client: StompClient) -> EventBusClient:

def test_context_manager_connects_and_disconnects(
events: EventBusClient,
mock_stomp_client: Mock,
mock_stomp_client: StompClient,
Copy link
Contributor

Choose a reason for hiding this comment

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

Honestly I never know what to do with type annotations for mocks. If you annotated with the mocked type you get code completion for its methods but you don't get assert_called_once et al. and vice versa.

I tend to annotate based on whether I'm using the real methods or the mock utilities more often but we should probably have a cohesive answer. I've tried using Mock(spec=ActualType) before but vscode doesn't seem to pick that up.

tests/unit_tests/worker/test_task_worker.py Show resolved Hide resolved
Comment on lines 67 to +69
with pytest.raises(KeyError) as exception:
client.get_plan("Not exists")
assert str(exception) == ("{'detail': 'Item not found'}")
assert str(exception) == ("{'detail': 'Item not found'}")
Copy link
Contributor

Choose a reason for hiding this comment

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

Could all these use raises(match='pattern') instead of having a separate assert? Something like

with pytest.raises(KeyError, match=NOT_FOUND):
    client.get_plan("Not exists")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, think this is done in #655- after that and #619 I'll rebase this and see what still needs to be done

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