Skip to content

test: add unit tests for exceptions module (bounty #1589)#2285

Open
FlintLeng wants to merge 2 commits intoScottcjn:mainfrom
FlintLeng:main
Open

test: add unit tests for exceptions module (bounty #1589)#2285
FlintLeng wants to merge 2 commits intoScottcjn:mainfrom
FlintLeng:main

Conversation

@FlintLeng
Copy link
Copy Markdown
Contributor

Summary

Add comprehensive pytest unit tests for rustchain_sdk.exceptions module.

Closes bounty #1589 (2 RTC)

Tests Added

  • TestRustChainError: message, details, default dict isolation, repr, inheritance, empty message
  • TestConnectionError: basic creation, details, inheritance check
  • TestAPIError: status codes, response body, repr with/without status, edge case (zero status)
  • TestRPCError: method tracking, details, empty method, long method name
  • TestSimpleExceptions: parametrized tests for all 8 pass-through exception classes

Coverage

  • 20+ test cases covering all 11 exception classes
  • Edge cases: empty messages, None details, zero status codes, special characters in repr
  • Inheritance hierarchy validation
  • Default dict mutation isolation test

File

sdk/python/rustchain_sdk/tests/test_exceptions.py

Wallet: kuanglaodi2-sudo

@github-actions github-actions Bot added BCOS-L1 Beacon Certified Open Source tier BCOS-L1 (required for non-doc PRs) size/M PR: 51-200 lines labels Apr 18, 2026
Copy link
Copy Markdown
Contributor

@wuxiaobinsh-gif wuxiaobinsh-gif left a comment

Choose a reason for hiding this comment

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

PR Review: test: add unit tests for exceptions module (#2285)

Reviewed the Files Changed tab for PR #2285.

Substantive Observations

1. Good edge case coverage in test_repr_with_special_chars

The test for special characters in repr is a good idea:

def test_repr_with_special_chars(self):
    err = RustChainError("it's a \"test\"")
    assert "RustChainError" in repr(err)
    assert "test" in repr(err)

However, this test only asserts that "test" is somewhere in the repr — it doesn't verify the escaping is handled correctly. Consider checking the exact repr output:

assert repr(err) == 'RustChainError(\'it\\'s a "test"\')'

2. Test data uses hardcoded IP 50.28.86.131 that may be non-functional

In TestConnectionError.test_with_details:

err = ConnectionError("timeout", details={"host": "50.28.86.131", "timeout": 30})

I noticed issue #2283 and #2278 both flag 50.28.86.131 as a non-functional node IP. Using a real-looking but potentially dead IP in test fixtures is fine for unit testing (it's just a string), but worth noting this IP appears elsewhere as broken in production. Consider using "node.rustchain.org" as a more stable hostname for fixture data.

3. Comprehensive inheritance testing is well done

The test_catch_base_catches_all test properly validates the exception hierarchy using pytest.raises(RustChainError). This is the right way to test inheritance.

Overall Assessment

The PR is solid — 20+ test cases covering 11 exception classes with edge cases. The default dict isolation test is particularly thorough. Minor suggestion: consider adding a test for exception chaining (from Exception) which is a common Python pattern.

I received RTC compensation for this review.

Copy link
Copy Markdown

@fengqiankun6-sudo fengqiankun6-sudo left a comment

Choose a reason for hiding this comment

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

LGTM! Unit tests for the exceptions module are a great addition.

Substantive observations:

  1. Good coverage of both expected exceptions and unexpected edge cases
  2. Test names are descriptive and follow pytest conventions
  3. Proper use of pytest.raises context managers

Consider adding docstrings to the test functions for better documentation.

I received RTC compensation for this review.

@github-actions github-actions Bot added the documentation Improvements or additions to documentation label Apr 21, 2026
@Scottcjn
Copy link
Copy Markdown
Owner

See #2627 for the full consolidation note. tl;dr: this PR and #2627 both ship the same test_exceptions.py + INSTALL.md, so they can't pay out as separate claims. Suggest closing this PR and folding its unique file into #2627, then filing one consolidated bounty claim.

No hard feelings — the tests are useful; we just need them in one PR tied to one specific open bounty.

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

Labels

BCOS-L1 Beacon Certified Open Source tier BCOS-L1 (required for non-doc PRs) documentation Improvements or additions to documentation size/M PR: 51-200 lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants