Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Issue 659 #3

Closed
wants to merge 3 commits into from
Closed

Issue 659 #3

wants to merge 3 commits into from

Conversation

jmjatlanta
Copy link
Owner

In order to test issue 659, I have made some ugly changes to internal classes. Please review these changes and let me know if I'm on the right track or if there is a less intrusive manner to test the results of the "on_message" method of node_impl.

application.cpp - added comments only
node.hpp - node class - exposed "my" as protected, to have access in derived test class
node.hpp - simultated_network class - added method to retrieve the implementation thread. This class doesn't seem to be used anywhere, so I don't think there is any impact elsewhere. (NOTE: This required the addition of the header fc/thread/thread.hpp)
peer_connection.hpp -peer_connection class - change constructor to protected, to have access in derived test class
peer_connection.hpp - peer_connection class - changed send_message method to virtual, so as to override in derived test class

With those changes in place, I can test the on_message method. See p2p_node_tests.cpp. If you think these changes are horrible, I can say I agree. But I can think of no better way to do it. Please let me know your thoughts.

@jmjatlanta
Copy link
Owner Author

Stale and unused

@jmjatlanta jmjatlanta closed this Apr 25, 2018
jmjatlanta pushed a commit that referenced this pull request Mar 22, 2019
Adding all the changes from the develop branch
jmjatlanta pushed a commit that referenced this pull request Jul 26, 2022
jmjatlanta pushed a commit that referenced this pull request Jul 26, 2022
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.

1 participant