Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix unit tests within active_transactions #3703

Merged
merged 3 commits into from
Feb 15, 2022

Conversation

theohax
Copy link
Contributor

@theohax theohax commented Feb 1, 2022

Fixed unit tests:

  • active_transactions.keep_local
  • active_transactions.fork_filter_cleanup
  • active_transactions.fifo

No issues to link to this PR (tests weren't disabled before).

@theohax theohax added bug unit test Related to a new, changed or fixed unit test labels Feb 1, 2022
@theohax theohax self-assigned this Feb 1, 2022
nano/core_test/active_transactions.cpp Outdated Show resolved Hide resolved
nano/core_test/active_transactions.cpp Show resolved Hide resolved
nano::state_block_builder builder;
auto & node1 = *system.add_node (node_config);
nano::keypair key{};
nano::send_block_builder builder{};
Copy link
Contributor

Choose a reason for hiding this comment

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

This also reverts to legacy blocks.

Copy link

Choose a reason for hiding this comment

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

perhaps it should be called legacy_send_block_builder to prevent others from making that mistake

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah it is a bit unfortunate -- I actually thought we're better off using send_, open_ etc builders since they're more specific, unbeknownst they refer to the legacy block types. Addressed, renaming might be a good idea too, any input on this @clemahieu ?

nano/core_test/active_transactions.cpp Outdated Show resolved Hide resolved
{
nano::vectorstream stream (block_bytes);
nano::vectorstream stream{ send_block_bytes };
send1->serialize (stream);
}

// Generate 10 forks to prevent new block insertion to election
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you determine why we need to prevent block insertion? It isn't immediately apparent how/why this is done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can't say either, let's bring it up

@zhyatt zhyatt added this to the V24.0 milestone Feb 1, 2022
@theohax theohax merged commit 13ba5c4 into develop Feb 15, 2022
@theohax theohax deleted the fix-unit-tests-active_transaction.xxx-TOPUSH branch February 15, 2022 14:54
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug unit test Related to a new, changed or fixed unit test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants