Skip to content

chore: Add unit tests for ToolboxSyncClient #225

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
May 16, 2025
Merged

chore: Add unit tests for ToolboxSyncClient #225

merged 8 commits into from
May 16, 2025

Conversation

anubhav756
Copy link
Contributor

@anubhav756 anubhav756 commented May 6, 2025 •
edited
Loading 

Name                              Stmts   Miss  Cover
-----------------------------------------------------
src/toolbox_core/__init__.py          3      0   100%
src/toolbox_core/client.py          102      0   100%
src/toolbox_core/protocol.py         32      0   100%
src/toolbox_core/sync_client.py      41      0   100%
src/toolbox_core/sync_tool.py        38      0   100%
src/toolbox_core/tool.py             95      0   100%
src/toolbox_core/utils.py            37      0   100%
-----------------------------------------------------

@anubhav756 anubhav756 self-assigned this May 6, 2025
@anubhav756 anubhav756 force-pushed the anubhav-sync-client-test branch from f5aaff6 to 4945c4f Compare May 7, 2025 11:12
@anubhav756 anubhav756 marked this pull request as ready for review May 7, 2025 11:12
@anubhav756 anubhav756 requested a review from a team as a code owner May 7, 2025 11:12
@anubhav756 anubhav756 force-pushed the anubhav-sync-client-test branch from 50d6e8d to 916c865 Compare May 7, 2025 11:25
@anubhav756 anubhav756 mentioned this pull request May 7, 2025
Merged
@anubhav756 anubhav756 changed the base branch from anubhav-sync-tool-test to anubhav-sync-client-loop May 8, 2025 11:05
@anubhav756 anubhav756 force-pushed the anubhav-sync-client-loop branch from 10be78b to 3d7d713 Compare May 12, 2025 14:13
@anubhav756 anubhav756 force-pushed the anubhav-sync-client-test branch from 916c865 to e08caae Compare May 12, 2025 14:13
kurtisvg
kurtisvg reviewed May 12, 2025
View reviewed changes
Copy link
Contributor

@kurtisvg kurtisvg left a comment

Choose a reason for hiding this comment

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

Just to confirm -- do we have a 1:1 mapping of sync tests to auth tests? I see some "Extra" sync tests. Should we move those into a Class so it's clear they are additive?

@anubhav756 anubhav756 force-pushed the anubhav-sync-client-loop branch from 3d7d713 to 2e77464 Compare May 13, 2025 10:32
@anubhav756
Copy link
Contributor Author

Just to confirm -- do we have a 1:1 mapping of sync tests to auth tests? I see some "Extra" sync tests. Should we move those into a Class so it's clear they are additive?

Yes, there is a 1:1 mapping for core auth tests in TestSyncAuth.

Thanks for the suggestion! Updated the sync client tests

  • Grouped the extra sync-specific lifecycle tests into a new TestSyncClientLifecycle class to make their additive nature clear.
  • Moved the existing sync header tests into TestSyncClientHeaders for better structure.

@anubhav756 anubhav756 force-pushed the anubhav-sync-client-test branch from e08caae to 91c2731 Compare May 13, 2025 11:26
@anubhav756 anubhav756 requested a review from kurtisvg May 13, 2025 11:36
@anubhav756 anubhav756 force-pushed the anubhav-sync-client-test branch from 91c2731 to 13079a1 Compare May 14, 2025 12:40
@anubhav756 anubhav756 force-pushed the anubhav-sync-client-loop branch from 2e77464 to f3a1865 Compare May 14, 2025 12:40
Base automatically changed from anubhav-sync-client-loop to main May 15, 2025 08:29
@anubhav756 anubhav756 force-pushed the anubhav-sync-client-test branch from 13079a1 to 81856a3 Compare May 15, 2025 08:29
anubhav756 added 8 commits May 15, 2025 14:16
@anubhav756
chore: Add unit test cases
85c7783
@anubhav756
chore: Delint
c4fcc90
@anubhav756
feat: Warn on insecure tool invocation with authentication
92ca91b 
This change introduces a warning that is displayed immediately before a tool invocation if:
1. The invocation includes an authentication header.
2. The connection is being made over non-secure HTTP.

> [!IMPORTANT]
The purpose of this warning is to alert the user to the security risk of sending credentials over an unencrypted channel and to encourage the use of HTTPS.
@anubhav756
fix!: Warn about https only during tool initialization
ad2dca0
@anubhav756
chore: Add unit tests for ToolboxSyncClient
60f86d8
@anubhav756
chore: Delint
f57cbc1
@anubhav756
chore: Refactor sync client tests to be grouped together in classes
a0ed437 
These also help identify differences with the existing async client tests.
@anubhav756
chore: Delint
b70dfa7
@anubhav756 anubhav756 force-pushed the anubhav-sync-client-test branch from 81856a3 to b70dfa7 Compare May 15, 2025 08:47
kurtisvg
kurtisvg approved these changes May 16, 2025
View reviewed changes
@anubhav756 anubhav756 merged commit 3516be5 into main May 16, 2025
19 checks passed
@anubhav756 anubhav756 deleted the anubhav-sync-client-test branch May 16, 2025 11:06
@release-please release-please bot mentioned this pull request May 16, 2025
Merged
@anubhav756 anubhav756 restored the anubhav-sync-client-test branch May 16, 2025 11:07
@anubhav756 anubhav756 mentioned this pull request May 16, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kurtisvg kurtisvg kurtisvg approved these changes

@twishabansal twishabansal Awaiting requested review from twishabansal

Assignees

@anubhav756 anubhav756

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@anubhav756 @kurtisvg