test: Don't include torcontrol.cpp into the test file by Empact · Pull Request #13291 · bitcoin/bitcoin

Empact · 2018-05-21T06:10:58Z

These methods are standalone string parsing methods which were included
into test via an include of torcontrol.cpp, which is bad practice.

Splitting them out reveals that they were the only torcontrol.cpp
methods under test, so the test file is renamed tor_reply_tests.cpp.

Introduced in #10408

practicalswift · 2018-05-21T07:19:56Z

Concept ACK

jonasschnelli · 2018-05-21T14:06:49Z

Wouldn't an extern definition of the function in test source not be sufficient and simpler?

maflcko · 2018-05-21T15:46:23Z

Agree with @jonasschnelli. I think just declaring the function in the test file should be sufficient and does indeed compile:

diff --git a/src/test/torcontrol_tests.cpp b/src/test/torcontrol_tests.cpp
index 8bd5ce1222..9ea085eaa7 100644
--- a/src/test/torcontrol_tests.cpp
+++ b/src/test/torcontrol_tests.cpp
@@ -3,11 +3,15 @@
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 //
 #include <test/test_bitcoin.h>
-#include <torcontrol.cpp>
+#include <torcontrol.h>
 
 #include <boost/test/unit_test.hpp>
 
 
+std::pair<std::string, std::string> SplitTorReplyLine(const std::string& s);
+std::map<std::string, std::string> ParseTorReplyMapping(const std::string& s);
+
+
 BOOST_FIXTURE_TEST_SUITE(torcontrol_tests, BasicTestingSetup)
 
 static void CheckSplitTorReplyLine(std::string input, std::string command, std::string args)
diff --git a/src/torcontrol.cpp b/src/torcontrol.cpp
index 717d1cf7e5..a4952a3336 100644
--- a/src/torcontrol.cpp
+++ b/src/torcontrol.cpp
@@ -251,7 +251,7 @@ bool TorControlConnection::Command(const std::string &cmd, const ReplyHandlerCB&
  * Grammar is implicitly defined in https://spec.torproject.org/control-spec by
  * the server reply formats for PROTOCOLINFO (S3.21) and AUTHCHALLENGE (S3.24).
  */
-static std::pair<std::string,std::string> SplitTorReplyLine(const std::string &s)
+std::pair<std::string, std::string> SplitTorReplyLine(const std::string& s)
 {
     size_t ptr=0;
     std::string type;
@@ -270,7 +270,7 @@ static std::pair<std::string,std::string> SplitTorReplyLine(const std::string &s
  * the server reply formats for PROTOCOLINFO (S3.21), AUTHCHALLENGE (S3.24),
  * and ADD_ONION (S3.27). See also sections 2.1 and 2.3.
  */
-static std::map<std::string,std::string> ParseTorReplyMapping(const std::string &s)
+std::map<std::string, std::string> ParseTorReplyMapping(const std::string& s)
 {
     std::map<std::string,std::string> mapping;
     size_t ptr=0;

Rather than including the implementation file into the test, which is bad practice.

Empact · 2018-05-21T17:56:54Z

SGTM, updated.

maflcko · 2018-05-21T21:36:43Z

utaCK 97c112d

practicalswift · 2018-05-21T21:43:53Z

utACK 97c112d

kallewoof

utACK 97c112d

97c112d Declare TorReply parsing functions in torcontrol_tests (Ben Woosley) Pull request description: These methods are standalone string parsing methods which were included into test via an include of torcontrol.cpp, which is bad practice. ~~Splitting them out reveals that they were the only torcontrol.cpp methods under test, so the test file is renamed tor_reply_tests.cpp.~~ Introduced in #10408 Tree-SHA512: 8ff11a9c900a88f910a73dfe16f43581a567e9d60e9298a8a963fc9dd7cffb4d97a644da677610aafb7d89f1dd1cede9afeae2c6344305e021a9a322dbcea0ac

Summary: Merge #10408: Net: Improvements to Tor control port parser 49a199b torcontrol: Handle escapes in Tor QuotedStrings (Jack Grigg) 0182a11 torcontrol: Log invalid parameters in Tor reply strings where meaningful (Jack Grigg) 0b6f40d torcontrol: Check for reading errors in ReadBinaryFile (Jack Grigg) d63677b torcontrol: Fix ParseTorReplyMapping (Jack Grigg) 29f3c20 torcontrol: Add unit tests for Tor reply parsers (Jack Grigg) d8e03c0 torcontrol: Improve comments (Jack Grigg) Tree-SHA512: aa3ce8072d20299b38c4ba9471af7fab1f5df096c237bf40a96ee9274a357f7366f95ced0cc80f8da1f22f6455a1a8e68bad9a5ff71817eef3397b6aefcbc7ae Backport of Core PR10408 https://github.com/bitcoin/bitcoin/pull/10408/files Merge #13291: test: Don't include torcontrol.cpp into the test file 97c112d Declare TorReply parsing functions in torcontrol_tests (Ben Woosley) Pull request description: These methods are standalone string parsing methods which were included into test via an include of torcontrol.cpp, which is bad practice. ~~Splitting them out reveals that they were the only torcontrol.cpp methods under test, so the test file is renamed tor_reply_tests.cpp.~~ Introduced in #10408 Tree-SHA512: 8ff11a9c900a88f910a73dfe16f43581a567e9d60e9298a8a963fc9dd7cffb4d97a644da677610aafb7d89f1dd1cede9afeae2c6344305e021a9a322dbcea0ac Backport of Core PR13291 bitcoin/bitcoin#13291 (partial) Merge #13163: Make it clear which functions that are intended to be translation unit local c3f34d0 Make it clear which functions that are intended to be translation unit local (practicalswift) Pull request description: Make it clear which functions that are intended to be translation unit local. Do not share functions that are meant to be translation unit local with other translation units. Use internal linkage for those consistently. Tree-SHA512: 05eebd233d5cfbf6116724eec3a99b465bf534ca220f2b6f5e56341a7da41387454d3cb6ceadd8ab6714a5df94069e4ad0dcab8801ccc7e8949be7199a19fb53 Backport of Core PR13163 bitcoin/bitcoin#13163 Completes T612 Introduces a memory leak fixed by PR10587 bitcoin/bitcoin#10587 Memory leak fix: https://reviews.bitcoinabc.org/D3285 Includes a fix to the locale linter discussed with fabien D3285 should be landed immediately after this one Test Plan: make check test_runner.py sudo apt install tor # if not already installed sudo etc/init.d/tor status # to check if tor is running sudo etc/init.d/tor start # if it is not already running ./src/bitcoind -proxy=127.0.0.1:<tor port> ./bitcoin-cli getpeerinfo tor defaults to port 9050. `getpeerinfo` should be populated. arc lint should pass with no errors. Reviewers: jasonbcox, deadalnix, Fabien, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: jasonbcox, deadalnix, Fabien, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D3283

Summary: Merge #10408: Net: Improvements to Tor control port parser 49a199b torcontrol: Handle escapes in Tor QuotedStrings (Jack Grigg) 0182a11 torcontrol: Log invalid parameters in Tor reply strings where meaningful (Jack Grigg) 0b6f40d torcontrol: Check for reading errors in ReadBinaryFile (Jack Grigg) d63677b torcontrol: Fix ParseTorReplyMapping (Jack Grigg) 29f3c20 torcontrol: Add unit tests for Tor reply parsers (Jack Grigg) d8e03c0 torcontrol: Improve comments (Jack Grigg) Tree-SHA512: aa3ce8072d20299b38c4ba9471af7fab1f5df096c237bf40a96ee9274a357f7366f95ced0cc80f8da1f22f6455a1a8e68bad9a5ff71817eef3397b6aefcbc7ae Backport of Core PR10408 https://github.com/bitcoin/bitcoin/pull/10408/files Merge #13291: test: Don't include torcontrol.cpp into the test file 97c112d4ca Declare TorReply parsing functions in torcontrol_tests (Ben Woosley) Pull request description: These methods are standalone string parsing methods which were included into test via an include of torcontrol.cpp, which is bad practice. ~~Splitting them out reveals that they were the only torcontrol.cpp methods under test, so the test file is renamed tor_reply_tests.cpp.~~ Introduced in #10408 Tree-SHA512: 8ff11a9c900a88f910a73dfe16f43581a567e9d60e9298a8a963fc9dd7cffb4d97a644da677610aafb7d89f1dd1cede9afeae2c6344305e021a9a322dbcea0ac Backport of Core PR13291 bitcoin/bitcoin#13291 (partial) Merge #13163: Make it clear which functions that are intended to be translation unit local c3f34d06be Make it clear which functions that are intended to be translation unit local (practicalswift) Pull request description: Make it clear which functions that are intended to be translation unit local. Do not share functions that are meant to be translation unit local with other translation units. Use internal linkage for those consistently. Tree-SHA512: 05eebd233d5cfbf6116724eec3a99b465bf534ca220f2b6f5e56341a7da41387454d3cb6ceadd8ab6714a5df94069e4ad0dcab8801ccc7e8949be7199a19fb53 Backport of Core PR13163 bitcoin/bitcoin#13163 Completes T612 Introduces a memory leak fixed by PR10587 bitcoin/bitcoin#10587 Memory leak fix: https://reviews.bitcoinabc.org/D3285 Includes a fix to the locale linter discussed with fabien D3285 should be landed immediately after this one Test Plan: make check test_runner.py sudo apt install tor # if not already installed sudo etc/init.d/tor status # to check if tor is running sudo etc/init.d/tor start # if it is not already running ./src/bitcoind -proxy=127.0.0.1:<tor port> ./bitcoin-cli getpeerinfo tor defaults to port 9050. `getpeerinfo` should be populated. arc lint should pass with no errors. Reviewers: jasonbcox, deadalnix, Fabien, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: jasonbcox, deadalnix, Fabien, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D3283

… file 97c112d Declare TorReply parsing functions in torcontrol_tests (Ben Woosley) Pull request description: These methods are standalone string parsing methods which were included into test via an include of torcontrol.cpp, which is bad practice. ~~Splitting them out reveals that they were the only torcontrol.cpp methods under test, so the test file is renamed tor_reply_tests.cpp.~~ Introduced in bitcoin#10408 Tree-SHA512: 8ff11a9c900a88f910a73dfe16f43581a567e9d60e9298a8a963fc9dd7cffb4d97a644da677610aafb7d89f1dd1cede9afeae2c6344305e021a9a322dbcea0ac

Empact changed the title ~~[moveonly] Extract tor_reply.h from tor_control.cpp~~ [moveonly] Extract tor_reply.h from torcontrol.cpp May 21, 2018

Empact force-pushed the tor-reply branch from 9de6d4e to e4bf5f3 Compare May 21, 2018 06:11

fanquake added the Refactoring label May 21, 2018

Empact force-pushed the tor-reply branch 3 times, most recently from 41f5acb to a7e6b59 Compare May 21, 2018 06:37

Empact mentioned this pull request May 21, 2018

rpc: Remove the need to include rpc/blockchain.cpp in order to put GetDifficulty under test #13288

Merged

Declare TorReply parsing functions in torcontrol_tests

97c112d

Rather than including the implementation file into the test, which is bad practice.

Empact force-pushed the tor-reply branch from a7e6b59 to 97c112d Compare May 21, 2018 17:55

Empact changed the title ~~[moveonly] Extract tor_reply.h from torcontrol.cpp~~ refactoring: Don't include torcontrol.cpp into the test file May 21, 2018

maflcko added the Tests label May 21, 2018

kallewoof approved these changes May 22, 2018

View reviewed changes

Empact mentioned this pull request May 22, 2018

lint: Add linter to error on #include <*.cpp> #13301

Merged

maflcko changed the title ~~refactoring: Don't include torcontrol.cpp into the test file~~ test: Don't include torcontrol.cpp into the test file May 24, 2018

maflcko merged commit 97c112d into bitcoin:master May 24, 2018

Bushstar mentioned this pull request May 26, 2018

commits from bitcoin/master FeatherCoin/Feathercoin#336

Merged

Empact deleted the tor-reply branch July 2, 2018 02:59

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: Don't include torcontrol.cpp into the test file#13291

test: Don't include torcontrol.cpp into the test file#13291
maflcko merged 1 commit intobitcoin:masterfrom
Empact:tor-reply

Empact commented May 21, 2018 •

edited

Loading

Uh oh!

practicalswift commented May 21, 2018

Uh oh!

jonasschnelli commented May 21, 2018

Uh oh!

maflcko commented May 21, 2018

Uh oh!

Empact commented May 21, 2018

Uh oh!

maflcko commented May 21, 2018

Uh oh!

practicalswift commented May 21, 2018

Uh oh!

kallewoof left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

Empact commented May 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented May 21, 2018

Uh oh!

jonasschnelli commented May 21, 2018

Uh oh!

maflcko commented May 21, 2018

Uh oh!

Empact commented May 21, 2018

Uh oh!

maflcko commented May 21, 2018

Uh oh!

practicalswift commented May 21, 2018

Uh oh!

kallewoof left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Empact commented May 21, 2018 •

edited

Loading