Skip to content

Conversation

@Giannis-dot
Copy link
Contributor

@Giannis-dot Giannis-dot commented Dec 10, 2025

…est running instructions

Description:
This PR adds a missing unit test for the subscription_handle method.
Ensures correct behavior when handling subscription IDs and validates expected output.

Related issue(s):

Fixes #

Notes for reviewer:

4 tests passed in 0.13s

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

…est running instructions

Signed-off-by: Giannis <giannisgeorgiadiis@gmail.com>
Copy link
Contributor

@tech0priyanshu tech0priyanshu left a comment

Choose a reason for hiding this comment

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

hi @Giannis-dot Thanks for time and effort. some change needs to address

Copy link
Contributor

@exploreriii exploreriii left a comment

Choose a reason for hiding this comment

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

Hi @Giannis-dot
Our tests will use pytest automatically when we trigger the existing workflows - I don't think there is a need to add your own workflow to test a particular file
All of them, even new ones added, should be added to the pytest test
Will you need ci.yml?

Copy link
Contributor

@exploreriii exploreriii left a comment

Choose a reason for hiding this comment

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

Also please don't forget to add a changelog entry :)

Changing this to draft while you apply changes

@exploreriii exploreriii marked this pull request as draft December 10, 2025 11:15
Signed-off-by: Giannis <giannisgeorgiadiis@gmail.com>
Signed-off-by: Giannis <giannisgeorgiadiis@gmail.com>
@Giannis-dot Giannis-dot marked this pull request as ready for review December 10, 2025 11:53
@Giannis-dot
Copy link
Contributor Author

@exploreriii i think i completed the changes, please check my new commits
Thanks

Signed-off-by: Giannis <giannisgeorgiadiis@gmail.com>
Copilot AI review requested due to automatic review settings December 10, 2025 12:11
@Giannis-dot
Copy link
Contributor Author

i made a branch mistake before. now the last commit-push is ready for review

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds comprehensive unit tests for the SubscriptionHandle class, testing its cancellation state management, thread lifecycle, and join operations. The PR also includes necessary test infrastructure improvements, including test shims for protobuf modules and a minimal mypy configuration file, along with documentation updates for local test execution.

  • Adds 4 unit tests covering the core functionality of SubscriptionHandle
  • Introduces test shims for protobuf/gRPC modules to enable isolated unit testing
  • Updates documentation with instructions for running tests locally

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tests/unit/test_subscription_handle.py Adds unit tests for SubscriptionHandle covering default state, cancellation, thread management, and join operations
tools/_mypy_minimal.ini Adds minimal mypy configuration for ignoring missing imports during testing
tests/_shims/hiero_sdk_python/hapi/services/timestamp_pb2.py Provides minimal shim for Timestamp protobuf class used in tests
tests/_shims/hiero_sdk_python/hapi/services/basic_types_pb2.py Provides minimal shim for basic protobuf types including TransactionID
tests/_shims/hiero_sdk_python/hapi/services/__init__.py Package initialization for services shims
tests/_shims/hiero_sdk_python/hapi/mirror/consensus_service_pb2_grpc.py Provides stub for gRPC consensus service used in tests
tests/_shims/hiero_sdk_python/hapi/mirror/__init__.py Package initialization for mirror shims
tests/_shims/hiero_sdk_python/hapi/__init__.py Package initialization documenting purpose of test shims
CONTRIBUTING.md Adds section on running tests locally with PowerShell example
CHANGELOG.md Documents addition of SubscriptionHandle unit tests

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@exploreriii exploreriii marked this pull request as draft December 10, 2025 12:57
Signed-off-by: Giannis <giannisgeorgiadiis@gmail.com>
Signed-off-by: Giannis <giannisgeorgiadiis@gmail.com>
Signed-off-by: Giannis <giannisgeorgiadiis@gmail.com>
@Giannis-dot
Copy link
Contributor Author

is everything ok?do i have to do something more?

Signed-off-by: GiannisGeo <giannisgeorgiadiis@gmail.com>
@Giannis-dot Giannis-dot marked this pull request as ready for review December 11, 2025 09:17
@Giannis-dot
Copy link
Contributor Author

@exploreriii hey, can you please review this to check if everything is ok?

@exploreriii exploreriii merged commit eab5ea7 into hiero-ledger:main Dec 11, 2025
14 checks passed
@exploreriii
Copy link
Contributor

congratulations @Giannis-dot !

@Giannis-dot
Copy link
Contributor Author

Thank you my friend, that was actually my first contribution. it was a pleasure

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