Skip to content

Conversation

@Subhrasameerdash
Copy link
Contributor

@Subhrasameerdash Subhrasameerdash commented Jan 24, 2026

Description:

Fixes #1522

Notes for reviewer:

Checklist

Copilot AI review requested due to automatic review settings January 24, 2026 17:08
@github-actions
Copy link

Hi @Subhrasameerdash, this is **LinkBot** 👋

Linking pull requests to issues helps us significantly with reviewing pull requests and keeping the repository healthy.

🚨 This pull request does not have an issue linked.

Please link an issue using the following format:

📖 Guide:
docs/sdk_developers/how_to_link_issues.md

If no issue exists yet, please create one:
docs/sdk_developers/creating_issues.md

Thanks!

@github-actions
Copy link

Hi, this is MergeConflictBot.
Your pull request cannot be merged because it contains merge conflicts.

Please resolve these conflicts locally and push the changes.

To assist you, please read:

Thank you for contributing!

@github-actions
Copy link

[commit-verification-bot]
Hi, this is VerificationBot.
Your pull request cannot be merged as it has unverified commits.
View your commit verification status: Commits Tab.

To achieve verified status, please read:

Remember, you require a GPG key and each commit must be signed with:
git commit -S -s -m "Your message here"

Thank you for contributing!

From the Hiero Python SDK Team

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 24, 2026

Walkthrough

The changes include formatting updates to the test configuration file using Black, addition of multiple new pytest fixtures for common test objects (IDs, credentials, mock client), and a changelog update documenting recent modifications including new documentation for a cryptography function.

Changes

Cohort / File(s) Summary
Documentation & Changelog
CHANGELOG.md
Added test formatting note for conftest.py (#1522) and comprehensive docstring for compress_with_cryptography function under new Changed section (#1626).
Test Fixtures
tests/unit/conftest.py
Added 10 new pytest fixtures (mock_account_ids, metadata, transaction_id, private_key, topic_id, nft_id, token_id, file_id, contract_id, mock_client) with updated mock_account_ids return value and mock_client NodeAddress formatting.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed Title accurately describes the main change: formatting tests/unit/conftest.py with Black and is directly related to the PR's primary objective.
Description check ✅ Passed Description clearly explains the changes: formatting conftest.py with Black and adding a CHANGELOG.md entry, both of which align with the PR objectives and linked issue.
Linked Issues check ✅ Passed The PR addresses issue #1522's primary requirements: formatting tests/unit/conftest.py with Black, adding a CHANGELOG entry, and includes signed commits and proper attribution.
Out of Scope Changes check ✅ Passed The changes are limited to the stated scope: formatting conftest.py with Black and updating CHANGELOG.md. No extraneous changes detected in the provided file summaries.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
tests/unit/conftest.py (1)

16-18: Remove duplicate import of TokenId.

TokenId is imported twice from the same module on lines 16 and 18. This is redundant and should be removed.

🔧 Proposed fix
 from hiero_sdk_python.tokens.token_id import TokenId
 from hiero_sdk_python.tokens.nft_id import NftId
-from hiero_sdk_python.tokens.token_id import TokenId
 from hiero_sdk_python.transaction.transaction_id import TransactionId

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 standardizes the formatting of tests/unit/conftest.py using Black and documents the change in the changelog, aligning with the repository’s ongoing test formatting cleanup.

Changes:

  • Reformatted tests/unit/conftest.py with Black (multi-line return, consistent quotes, spacing, and trailing commas).
  • Added a changelog entry under “Tests” describing the new formatting for tests/unit/conftest.py.

Reviewed changes

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

File Description
tests/unit/conftest.py Reformats test fixtures to Black style (whitespace, line breaks, trailing commas, and quote consistency) without changing runtime behavior.
CHANGELOG.md Adds a “Tests” section entry documenting the Black formatting of tests/unit/conftest.py.

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

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 @Subhrasameerdash thanks for asking for help!
Your commits are unverified because while you are signing with your GPG key, you forgot to tie it to github.

you can see that when you hover over the verified badge

Image

@exploreriii
Copy link
Contributor

You can use the online editor to resolve the changelog conflict, simply, accept both changes

Click the resolve conflcits button here on the PR!
Uploading Screenshot 2026-01-24 at 18.03.45.png…

@exploreriii exploreriii marked this pull request as draft January 24, 2026 18:04
@Subhrasameerdash Subhrasameerdash marked this pull request as ready for review January 25, 2026 04:23
@Subhrasameerdash Subhrasameerdash marked this pull request as draft January 25, 2026 04:24
@Subhrasameerdash

This comment was marked as resolved.

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.

Yes! You got close!
Please note your commits remain unverified because you have signed the commits with a different key id

Note here
https://github.com/hiero-ledger/hiero-sdk-python/pull/1584/commits

Hover over the verified and it will tell you what key id it expects, compare that to the key id on github

I recommend you soft revert your commits:
git reset --soft HEAD~2
ensure you use the GPG key tied to Github
recommit

git commit -S -s -m "chore: format conftest with black"

Any merge conflict can be solved on the online editor in the PR by accepting both changes :)

@Subhrasameerdash Subhrasameerdash force-pushed the chore/format-conftest-with-black branch from 7ca7f5c to 106a162 Compare January 25, 2026 06:30
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.

@exploreriii exploreriii requested a review from a team January 25, 2026 06:53
@Subhrasameerdash Subhrasameerdash marked this pull request as ready for review January 25, 2026 07:18
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

@prajeeta15
Copy link
Contributor

prajeeta15 commented Jan 25, 2026

@Subhrasameerdash just address the changes requested and duplicate entries in CHANGELOG.md , everything else LGTM !

@Subhrasameerdash Subhrasameerdash marked this pull request as draft January 25, 2026 08:19
@Subhrasameerdash Subhrasameerdash force-pushed the chore/format-conftest-with-black branch from 9b5205a to 397ff7e Compare January 25, 2026 08:46
@Subhrasameerdash Subhrasameerdash marked this pull request as ready for review January 25, 2026 08:48
@Subhrasameerdash
Copy link
Contributor Author

@prajeeta15 I have made the required changes. Please check and verify

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
tests/unit/conftest.py (1)

28-43: Scope creep vs. formatting-only objective.

Issue #1522 requires formatting-only changes, but this PR adds new fixtures and behaviors. Please confirm this is intended, or split functional additions into a separate PR to meet the acceptance criteria.

♻️ Duplicate comments (1)
CHANGELOG.md (1)

15-20: Remove duplicate changelog entries.

Line 15 duplicates Line 17, and Line 16 duplicates Line 20. Please keep only one entry per change.

🧾 Suggested fix
- Format `tests/unit/network_tls_test.py` with black for code style consistency (`#1543`)
- Format `tests/unit/conftest.py` with black for code style consistency. (`#1522`)
- Format tests/unit/network_tls_test.py with black for code style consistency (`#1543`)
+ Format `tests/unit/network_tls_test.py` with black for code style consistency (`#1543`)
+ Format `tests/unit/conftest.py` with black for code style consistency. (`#1522`)
...
- Format `tests/unit/conftest.py` with black for code style consistency. (`#1522`)

@prajeeta15
Copy link
Contributor

@Subhrasameerdash please look into the coderabbitai issues.
and there is still a duplicate entry in changelog.md

@aceppaluni
Copy link
Contributor

@Subhrasameerdash Thank you so much for your contribution! This is truly great work!!

Please be sure to address merge conflicts.
If you need assistance let us know we'd be happy to.

We are excited to see contributors wanting to take on more advanced tasks.
I have opened up a beginner issue and am linking it here, #1449
If you would like to take it on, leave a comment and you will be assigned.

We look forward to seeing more contributions and thank you!

@codecov
Copy link

codecov bot commented Jan 29, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1584   +/-   ##
=======================================
  Coverage   92.89%   92.89%           
=======================================
  Files         140      140           
  Lines        8765     8765           
=======================================
  Hits         8142     8142           
  Misses        623      623           
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Subhrasameerdash
Copy link
Contributor Author

@Subhrasameerdash hey! could you please resolve the final merge conflicts so it can be ready to be merged :)

I have updated the merge conflicts

@Subhrasameerdash
Copy link
Contributor Author

@Subhrasameerdash Thank you so much for your contribution! This is truly great work!!

Please be sure to address merge conflicts. If you need assistance let us know we'd be happy to.

We are excited to see contributors wanting to take on more advanced tasks. I have opened up a beginner issue and am linking it here, #1449 If you would like to take it on, leave a comment and you will be assigned.

We look forward to seeing more contributions and thank you!

Thank you for the feedback and encouragement!

I have resolved the merge conflicts.

I'm also interested in tackling #1449 - I'll comment on that issue to get assigned.

Thanks again for the opportunity to contribute!

exploreriii
exploreriii previously approved these changes Jan 30, 2026
@exploreriii
Copy link
Contributor

Hi @Subhrasameerdash please resolve the conflicts, then this is ready to go 👍

@exploreriii exploreriii marked this pull request as draft January 30, 2026 03:16
@Subhrasameerdash Subhrasameerdash marked this pull request as ready for review January 30, 2026 12:22
@Subhrasameerdash
Copy link
Contributor Author

Hii @exploreriii i have resolved the conflicts

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
tests/unit/node_tls_test.py (1)

357-365: ⚠️ Potential issue | 🔴 Critical

Fix malformed with-block in certificate-missing test.

The unindented line inside the with pytest.raises(...) block is a SyntaxError. Keep the new enabled=True call and remove the stray line.

🛠️ Proposed fix
 def test_secure_connect_raise_error_if_no_certificate_is_available(
     mock_node_without_address_book,
 ):
     """Test get channel raise error if no certificate available if transport security true."""
     node = mock_node_without_address_book
     node._apply_transport_security(enabled=True)
     with pytest.raises(ValueError, match=r"No certificate available\."):
-    node._apply_transport_security(True)
-
-    with pytest.raises(ValueError, match="No certificate available."):
         node._get_channel()

Copy link
Contributor

@prajeeta15 prajeeta15 left a comment

Choose a reason for hiding this comment

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

@Subhrasameerdash please look into the duplicate parameters.

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 @Subhrasameerdash
Thank you for this PR
However, I cannot accept the PR until it satisfies the issue description

The issue was just to format conftest.py, but here in your PR I see changes to node_tls_test.py
https://github.com/hiero-ledger/hiero-sdk-python/pull/1584/changes#r2746926549

Please revert all non-relevant changes and then request a review again 👍
Thank you for your understanding

@exploreriii exploreriii marked this pull request as draft January 30, 2026 16:01
@Subhrasameerdash Subhrasameerdash marked this pull request as ready for review January 30, 2026 17:43
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
tests/unit/node_tls_test.py (1)

354-365: ⚠️ Potential issue | 🔴 Critical

Critical syntax error - test file will not parse.

The test has invalid Python syntax that prevents pytest from collecting this file:

  • Line 360 opens a with pytest.raises(...) block, but line 361 (node._apply_transport_security(True)) is not indented under it
  • Python raises IndentationError: expected an indented block after 'with' statement
  • Lines 359-361 appear to be an incomplete/duplicate version overlapping with the intended test at lines 363-364

Remove the redundant lines 360-362 and keep only the properly indented version:

Fix
 def test_secure_connect_raise_error_if_no_certificate_is_available(
     mock_node_without_address_book,
 ):
     """Test get channel raise error if no certificate available if transport security true."""
     node = mock_node_without_address_book
     node._apply_transport_security(enabled=True)
-    with pytest.raises(ValueError, match=r"No certificate available\."):
-    node._apply_transport_security(True)
 
     with pytest.raises(ValueError, match=r"No certificate available\."):
         node._get_channel()

@Subhrasameerdash
Copy link
Contributor Author

Hi @Subhrasameerdash Thank you for this PR However, I cannot accept the PR until it satisfies the issue description

The issue was just to format conftest.py, but here in your PR I see changes to node_tls_test.py https://github.com/hiero-ledger/hiero-sdk-python/pull/1584/changes#r2746926549

Please revert all non-relevant changes and then request a review again 👍 Thank you for your understanding

Hi @exploreriii I have made the necessary changes.
I apologize for the mess up and thank you for being so supportive ☺️

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.

This is still showing many changes to node_tls_test.py
https://github.com/hiero-ledger/hiero-sdk-python/pull/1584/changes

try to soft revert your commits then only commit again the conftet and changelog, discard any change to node tls test

git reset --soft HEAD~19
git commit -S -s -m "chore: format conftest"
git commit -S -s -m "chore: format conftest changelog entry"
git push --force

@exploreriii exploreriii marked this pull request as draft January 31, 2026 04:47
@Subhrasameerdash Subhrasameerdash force-pushed the chore/format-conftest-with-black branch from a01df0f to 9cbc175 Compare January 31, 2026 14:02
@Subhrasameerdash Subhrasameerdash marked this pull request as ready for review January 31, 2026 14:05
Copy link

@rwalworth rwalworth left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks @Subhrasameerdash for your contribution. Recommend merging @exploreriii

@exploreriii exploreriii merged commit 2534698 into hiero-ledger:main Feb 1, 2026
22 of 23 checks passed
@github-actions
Copy link

github-actions bot commented Feb 1, 2026

🎉 Nice work completing a Good First Issue!

Thank you for your contribution to the Hiero Python SDK! We're excited to have you as part of our community.

Here are some issues labeled Beginner you might be interested in working on next:

  1. [Beginner]: Implement __repr__ Method for FileId Class

    🐥 Beginner Friendly This issue is intended for contributors who have previously completed a Good First Issue in Hiero and are at a beginner level....

  2. [Beginner]: Implement __repr__ Method for NftId Class

    🐥 Beginner Friendly This issue is intended for contributors who have previously completed a Good First Issue in Hiero and are at a beginner level....

🌟 Stay connected with the project:

We look forward to seeing more contributions from you! If you have any questions, feel free to ask in our Discord community.

From the Hiero Python SDK Team 🚀

@Subhrasameerdash Subhrasameerdash deleted the chore/format-conftest-with-black branch February 1, 2026 12:29
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.

[Good First Issue]: format tests/unit/conftest.py

7 participants