Improves recently added unit tests.
http_server_socket_tests, it was incorrectly referring to unordered_maphttp_request_state_tests unit test method use Uppercase as per developer-notes.md, shorten enum values for readability
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/36160.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline and AI policy for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | winterrdog, janb84, pinheadmz, sedited |
| Concept ACK | jeanpablojp |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, this pull request conflicts with the following ones:
HTTPRequest update state internally by hodlinator)If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
891 | - BOOST_CHECK(actual.find("Date: Wed, 11 Dec 2024 00:47:09 GMT\r\n") != std::string::npos); 892 | + BOOST_CHECK_EQUAL(actual, "HTTP/1.1 200 OK\r\n" 893 | + "Date: Wed, 11 Dec 2024 00:47:09 GMT\r\n" 894 | + "Content-Length: 7\r\n" 895 | + "Content-Type: text/html; charset=ISO-8859-1\r\n" 896 | + "Connection: close\r\n"
NIT: Although the suggested code in the PR is already a great improvement, I would suggest to hoist the expected string to the top. This remove some magic numbers (the reserve of 146 bytes with 10ms sleep is redundant) and the actual.length can be compared with the expected.length. And removes some risk of code drift (between the loop and expectation)
const std::string expected{"HTTP/1.1 200 OK\r\n"
"Date: Wed, 11 Dec 2024 00:47:09 GMT\r\n"
"Content-Length: 7\r\n"
"Content-Type: text/html; charset=ISO-8859-1\r\n"
"Connection: close\r\n"
"\r\n874140\n"};
std::string actual;
// Wait up to one minute for all the bytes to appear in the "send" pipe.
char buf[0x10000] = {};
attempts = 6000;
while (attempts > 0)
{
ssize_t bytes_read = mock_client_socket_pipes->send.GetBytes(buf, sizeof(buf), 0);
if (bytes_read > 0) {
actual.append(buf, bytes_read);
if (actual.length() >= expected.length()) {
break;
}
}
std::this_thread::sleep_for(10ms);
--attempts;
}
BOOST_CHECK_EQUAL(actual, expected);
Thanks for the feedback! Addressed in latest push.
Concept ACK e69331c329877f824ac06a92b00abff116125bad
Great improvement in readability! using enum matches the existing usage in the other files.
Found some typo's in the commit messages:
In commit message of 8b1c375a85a36118388e88c56bdc2b8543c7fc4f where -> were
In commit message of 'e69331c329877f824ac06a92b00abff116125bad' incresae -> increases
During the development of the new HTTP server, the headers were initially stored in an unordered_map, but later it was changed to a vector.
The former conforms to developer-notes.md, the latter increases readability.
tACK f04b0c31627adda1d54b7da4507cfe30a63f7e5e
successfully built and tested on this toolchain: Debian/clang++ 18/x86_64.
Concept ACK
Confirmed the old checks still pass after swapping two headers in WriteReply, so they were blind rather than tolerant.
483 | @@ -484,50 +484,51 @@ BOOST_AUTO_TEST_CASE(http_request_state_tests) 484 | public: 485 | DummyClient() : HTTPRemoteClient{/*id=*/0, /*addr=*/CService(), /*socket=*/CreateSock(0, 0, 0)} {} 486 | 487 | - void receive(std::string_view s) 488 | + void Receive(std::string_view s)
nit: Could we call this helper AppendToRecvBuffer? The name Receive hides HTTPRemoteClient's no-argument method, while this helper only appends data to the buffer.
Holding off on this for 4 reasons:
Receive() in #36135 tooHTTPRemoteClient::Receive() to something more descriptive in a follow-up PR after HTTP code churn settles, see #36135 (review)
ACK f04b0c31627adda1d54b7da4507cfe30a63f7e5e
LGTM.
ACK f04b0c31627adda1d54b7da4507cfe30a63f7e5e
Built and tested on macos/arm64, reviewed changes. Nice cleanup without changing any major logic. Thanks!
<details><summary>Show Signature</summary>
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256
ACK f04b0c31627adda1d54b7da4507cfe30a63f7e5e
-----BEGIN PGP SIGNATURE-----
iQJPBAEBCAA5FiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmqpjy4bFIAAAAAABAAO
bWFudTIsMi41KzEuMTIsMCwzAAoJEOfimEtiick6uqEP+QEImrgKI6bSztoCps6t
Wg9SV3N5ddec1YH+V5OXx7NB4TM3UwKNbIEyWa3zZPOyovC3VuXwYp1kd+H1Nwa1
oIqq1NRm8dmuQtDXHhuLRnWpc6Cu9pap0k4L87K7JNGUfHmb84vmbv5Kce00mqeE
PSFduWKjcPXj19GLOUkDLqVG0vN8W322TrAv//sn1cpdSkyo4mf5ddnQ7D3vDJKP
2zGlhownAHuYlDeC0WsqufS4RHBPMVtlIw16RnW33UF/7GupwUfnjpalhDiAvRz6
JCx3rTTijzGjz3uUxX40vzNYc5KHuRtCwnaaHNZwo+SecQ2cITNAk567cn+XgIsU
cQE3blu8Euagr5VJZuVdO7GPzUkX5Rf2k3KhNhv6T60mFlFkL1lHcNFKplAIl9WH
EMjmX1bS3tii6UGySHmkoWKiQ7nvxvJiMRLYDSFuX+kcwkcHThbhO8xQXVnUSioJ
Ved0kblavuMIC08bG+n+mGHeYMs9lRxst+5Ji9BidIh8xXoNBMFQtBOycbqguZ+O
R1AtgDiuJkaRcD5JIUbU2b8CMLjGRkoh5AY20qCz9WDGFy4CJKPl2yX4QzWAyhgl
kW3pppcPrKlNbWRvh/IMmjifiyj4LbL3bge3cucTHODb+Tn80VfXspIxxY2wVkLb
XcXqeJO8KYWCnuZbsBXgqpg5
=dTB3
-----END PGP SIGNATURE-----
pinheadmz's public key is on openpgp.org
</details>
ACK f04b0c31627adda1d54b7da4507cfe30a63f7e5e