Skip to content

Advanced tests Suites 1 & 2#34

Merged
Ignacio-87 merged 28 commits intodevfrom
advanced_tests
Feb 10, 2026
Merged

Advanced tests Suites 1 & 2#34
Ignacio-87 merged 28 commits intodevfrom
advanced_tests

Conversation

@rafanog
Copy link
Copy Markdown
Contributor

@rafanog rafanog commented Jan 19, 2026

Added 2 suites of advanced tests

@rafanog rafanog requested a review from Ignacio-87 January 19, 2026 12:29
@rafanog rafanog self-assigned this Jan 19, 2026
@rafanog rafanog changed the base branch from main to dev January 19, 2026 15:16
Copy link
Copy Markdown
Contributor

@Ignacio-87 Ignacio-87 left a comment

Choose a reason for hiding this comment

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

Request changes:

  • We need to merge dev into this.
  • Some test are not runnig
  • Remove ignore attribute in each tests.
  • Please remove the automock library and the trait created to test with automock

impl<B> IndexerApi for Indexer<B>
where
    B: BitcoinClientApi,
    

Copy link
Copy Markdown
Contributor

@Ignacio-87 Ignacio-87 left a comment

Choose a reason for hiding this comment

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

Please remove the mock library and replace all tests that are using mock with a real Bitcoin client.

@rafanog rafanog requested a review from Ignacio-87 January 23, 2026 18:31
@rafanog
Copy link
Copy Markdown
Contributor Author

rafanog commented Jan 27, 2026

All mock for bitcoin client and IndexerAPI were removed, and tests adjusted as requested.

@rafanog rafanog requested a review from Ignacio-87 January 27, 2026 14:07
@rafanog rafanog requested a review from Ignacio-87 January 28, 2026 11:10
@rafanog rafanog requested a review from Ignacio-87 January 30, 2026 21:03
Copy link
Copy Markdown
Contributor

@Ignacio-87 Ignacio-87 left a comment

Choose a reason for hiding this comment

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

In your last commit where you reimplemented the tests for estimate_fee_rate, there are several issues.

First, estimate_fee_rate is never actually called.
Second, the block-filling logic is still relying on mocks. No real transactions are being sent to the blockchain.
Finally, the estimate_fee_rate value is not being computed from the indexer.

Let me know if you want me to clarify any of these points.

@rafanog
Copy link
Copy Markdown
Contributor Author

rafanog commented Feb 2, 2026

The updated tests now properly verify your three requirements:

1 - estimate_fee_rate is called
All three tests explicitly call estimate_fee_rate():
-test_estimate_fee_rate_with_real_transactions - Line 511
-test_estimate_fee_rate_with_few_transactions - Line 567
-test_estimate_fee_rate_returns_valid_result - Line 589

2 - Real transactions from blockchain (not mocks)
Removed all mock transaction creation - no more fake BlockInfo or Transaction objects
Tests now mine actual blocks on regtest Bitcoin node using mine_blocks_to_address()

test_estimate_fee_rate_with_real_transactionscreates multiple real addresses and mines blocks to them
All tests verify blocks contain real transactions: assert!(block.txs.len() > 0) and check for actual coinbase transactions

Tests explicitly assert they're using real blockchain data: "Block should have transactions from real blockchain"

3 - Fee rate computed from indexer
test_estimate_fee_rate_computation_from_indexer specifically tests this:
-Creates a full Indexer instance with real storage
-Processes blocks through indexer.tick() - this triggers estimate_fee_rate() internally
-Retrieves the stored block from IndexerStore
-Verifies the estimated_fee_rate field was computed and stored by the indexer
-This proves the fee rate flows through the entire indexer pipeline: BitcoinClientestimate_fee_rate()IndexerStoreFullBlock

@rafanog rafanog requested a review from Ignacio-87 February 3, 2026 11:22
@danielemiliogarcia
Copy link
Copy Markdown
Contributor

For test_estimate_fee

I think we should add a test similar to test_estimate_fee_rate_with_real_transactions that inject more than 5 transactions into a block to force a fee rate different from zero, similar to the original test_get_estimated_fee_rate_with_seven_transactions but aimed to the private function that only returns a number, not to the API func that translate to errors

@rafanog
Copy link
Copy Markdown
Contributor Author

rafanog commented Feb 4, 2026

For test_estimate_fee

I think we should add a test similar to test_estimate_fee_rate_with_real_transactions that inject more than 5 transactions into a block to force a fee rate different from zero, similar to the original test_get_estimated_fee_rate_with_seven_transactions but aimed to the private function that only returns a number, not to the API func that translate to errors

The original test was removed due to the need of removing/adapting the logic implicating the use of MockBitcoinClient and since there seems to be no way to batch multiple transactions in the same block in the real bitcoinclient, this test can't be implemented.

@Ignacio-87 @danielemiliogarcia I can reinstate the test but only by using a MockBitcoinclient since it allows to prepare the scenario for it, but I'd like both your approval for this.

@danielemiliogarcia
Copy link
Copy Markdown
Contributor

For test_estimate_fee
I think we should add a test similar to test_estimate_fee_rate_with_real_transactions that inject more than 5 transactions into a block to force a fee rate different from zero, similar to the original test_get_estimated_fee_rate_with_seven_transactions but aimed to the private function that only returns a number, not to the API func that translate to errors

The original test was removed due to the need of removing/adapting the logic implicating the use of MockBitcoinClient and since there seems to be no way to batch multiple transactions in the same block in the real bitcoinclient, this test can't be implemented.

@Ignacio-87 @danielemiliogarcia I can reinstate the test but only by using a MockBitcoinclient since it allows to prepare the scenario for it, but I'd like both your approval for this.

Let's try to reproduce the same test effect using regtest instead of the mock

@rafanog
Copy link
Copy Markdown
Contributor Author

rafanog commented Feb 6, 2026

For test_estimate_fee
I think we should add a test similar to test_estimate_fee_rate_with_real_transactions that inject more than 5 transactions into a block to force a fee rate different from zero, similar to the original test_get_estimated_fee_rate_with_seven_transactions but aimed to the private function that only returns a number, not to the API func that translate to errors

The original test was removed due to the need of removing/adapting the logic implicating the use of MockBitcoinClient and since there seems to be no way to batch multiple transactions in the same block in the real bitcoinclient, this test can't be implemented.
@Ignacio-87 @danielemiliogarcia I can reinstate the test but only by using a MockBitcoinclient since it allows to prepare the scenario for it, but I'd like both your approval for this.

Let's try to reproduce the same test effect using regtest instead of the mock

I've added a couple of tests to cover the mentioned scenario, please @danielemiliogarcia @Ignacio-87 review this and give feedback if any further changes are required

@Ignacio-87 Ignacio-87 merged commit a1670cd into dev Feb 10, 2026
1 check passed
@Ignacio-87 Ignacio-87 deleted the advanced_tests branch February 10, 2026 17:52
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