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

Clarify/Improve test_some test #922

Merged
merged 1 commit into from
Jun 22, 2018
Merged

Clarify/Improve test_some test #922

merged 1 commit into from
Jun 22, 2018

Conversation

hugbubby
Copy link
Member

@hugbubby hugbubby commented Jun 21, 2018

Same thing as last time, another mild pull request as I go. A few better error messages, more logical sleep() call placements, comments, etc. For one of the test_basic functions I randomized the time between messages and the sizes in which portions of message are sent, for more coverage.

Did not modify large chunk of the test_some function because I didn't understand the second half of it. Maybe I'll come back later once I've figured out what it's doing.


This change is Reviewable

@iphydf iphydf added this to the v0.2.3 milestone Jun 22, 2018
@iphydf
Copy link
Member

iphydf commented Jun 22, 2018

Review status: 0 of 1 LGTMs obtained


auto_tests/TCP_test.c, line 110 at r1 (raw file):

                  "The attempt to send the last byte of handshake failed.");

    c_sleep(100);

Why 100?


auto_tests/TCP_test.c, line 118 at r1 (raw file):

    uint8_t response_plain[TCP_HANDSHAKE_PLAIN_SIZE];
    ck_assert_msg(net_recv(sock, response, TCP_SERVER_HANDSHAKE_SIZE) == TCP_SERVER_HANDSHAKE_SIZE,
                  "Could/Did not receive a server response to the initial handshake.");

nit: "did" lowercase.


auto_tests/TCP_test.c, line 140 at r1 (raw file):

    uint32_t i = 0;

    while (i < sizeof(r_req)) {

for (uint32_t i = 0; i < sizeof(r_req);) { ...

This limits the scope of i to the loop.


auto_tests/TCP_test.c, line 141 at r1 (raw file):

    while (i < sizeof(r_req)) {
        uint8_t msg_length = rand() % 5 + 1; // msg_length = 1-5

1-5 = -4. What you mean is: msg_length is between 1 and 5.


auto_tests/TCP_test.c, line 151 at r1 (raw file):

        i += msg_length;

        c_sleep(rand() % 10 + 45); // Wait 45-55 milliseconds

Why the random jitter?


auto_tests/TCP_test.c, line 155 at r1 (raw file):

    }

    // Receiving the second resonse and verifying its validity

"response"


auto_tests/TCP_test.c, line 173 at r1 (raw file):

    ck_assert_msg(public_key_cmp(packet_resp_plain + 2, f_public_key) == 0, "Server sent the wrong public key.");

    //Closing connections.

nit: space after //


auto_tests/TCP_test.c, line 316 at r1 (raw file):

    ck_assert_msg(public_key_cmp(data + 2, con1->public_key) == 0, "Key in response packet wrong.");

    uint8_t test_packet[512] = {16, 17, 16, 86, 99, 127, 255, 189, 78}; //What is this packet????

No idea :). @irungentoo: do you remember what this packet means?


Comments from Reviewable

@hugbubby
Copy link
Member Author

hugbubby commented Jun 22, 2018

Review status: 0 of 1 LGTMs obtained


auto_tests/TCP_test.c, line 110 at r1 (raw file):

Previously, iphydf wrote…

Why 100?

Old file did it with two calls and I just ended up merging them.


auto_tests/TCP_test.c, line 140 at r1 (raw file):

Previously, iphydf wrote…

for (uint32_t i = 0; i < sizeof(r_req);) { ...

This limits the scope of i to the loop.

Done.


auto_tests/TCP_test.c, line 141 at r1 (raw file):

Previously, iphydf wrote…

1-5 = -4. What you mean is: msg_length is between 1 and 5.

Done.


auto_tests/TCP_test.c, line 151 at r1 (raw file):

Previously, iphydf wrote…

Why the random jitter?

Idk, indecipherably useless idea to RANDOMIZE TIME BETWEEN SENDS

EXTRA DURABILITY


auto_tests/TCP_test.c, line 155 at r1 (raw file):

Previously, iphydf wrote…

"response"

Done.


auto_tests/TCP_test.c, line 316 at r1 (raw file):

Previously, iphydf wrote…

No idea :). @irungentoo: do you remember what this packet means?

Imagine being such a chad you directly read and write packets in raw uint8 arrays


Comments from Reviewable

@iphydf
Copy link
Member

iphydf commented Jun 22, 2018

:lgtm_strong:


Reviewed 1 of 1 files at r2.
Review status: :shipit: complete! 1 of 1 LGTMs obtained


Comments from Reviewable

Better error messages, better sleep() call placements, etc.
Did not modify large chunk of function because I couldn't explain
it. Maybe I'll come back later once I've regained lost brain cells.
@iphydf iphydf merged commit 36e20e7 into TokTok:master Jun 22, 2018
iphydf added a commit to iphydf/c-toxcore that referenced this pull request Sep 5, 2018
v0.2.3
**Merged PRs:**

- [TokTok#951] Only run astyle if the astyle binary exists.
- [TokTok#950] Remove utils.c and utils.h from toxencryptsave build.
- [TokTok#949] Fixes to the imported sodium sources to compile without warnings.
- [TokTok#948] Add a MAX_HOSTNAME_LENGTH constant.
- [TokTok#947] Remove the format test.
- [TokTok#937] Add new Circle CI configuration.
- [TokTok#935] Add a test for double conference invite.
- [TokTok#933] Add Logger to various net_crypto functions, and add `const` to Logger where possible.
- [TokTok#931] Avoid conditional-uninitialised warning for tcp test.
- [TokTok#930] Disable UDP when proxy is enabled.
- [TokTok#928] Use clang-format for C++ code.
- [TokTok#927] Add assertions to bootstrap tests for correct connection type.
- [TokTok#926] Make NULL options behave the same as default options.
- [TokTok#925] Add tests for what happens when passing an invalid proxy host.
- [TokTok#924] Make the net_crypto connection state an enum.
- [TokTok#922] Clarify/Improve test_some test
- [TokTok#921] Beginnings of a TCP_test.c overhaul
- [TokTok#920] Add test for creating multiple conferences in one tox.
- [TokTok#918] Merge irungentoo/master into toktok
- [TokTok#917] Add random testing program.
- [TokTok#916] Fix linking with address sanitizer.
- [TokTok#915] Remove resource_leak_test.
- [TokTok#914] Make dht_test more stable.
- [TokTok#913] Minor cleanup: return early on error condition.
- [TokTok#906] Sort bazel build file according to buildifier standard.
- [TokTok#905] In DEBUG mode, make toxcore crash on signed integer overflow.
- [TokTok#902] Log only the filename, not the full path in LOGGER.
- [TokTok#899] Fix macOS macro because of GNU Mach
- [TokTok#898] Fix enumeration of Crypto_Connection instances
- [TokTok#897] Fix ipport_isset: port 0 is not a valid port.
- [TokTok#894] Fix logging related crash in bootstrap node
- [TokTok#893] Fix bootstrap crashes, still
- [TokTok#892] Add empty logger to DHT bootstrap daemons.
- [TokTok#887] Fix FreeBSD build on Travis
- [TokTok#884] Fix the often call of event tox_friend_connection_status
- [TokTok#883] Make toxcore compile on BSD
- [TokTok#878] fix DHT_bootstrap key loading
- [TokTok#877] Add minitox to under "Other resources" section in the README
- [TokTok#875] Make bootstrap daemon use toxcore's version
- [TokTok#867] Improve network error reporting on Windows
- [TokTok#841] Only check full rtp offset if RTP_LARGE_FRAME is set
- [TokTok#823] Finish @Diadlo's network Family abstraction.
- [TokTok#822] Move system header includes from network.h to network.c
iphydf added a commit to iphydf/c-toxcore that referenced this pull request Sep 5, 2018
v0.2.3
**Merged PRs:**

- [TokTok#951] Only run astyle if the astyle binary exists.
- [TokTok#950] Remove utils.c and utils.h from toxencryptsave build.
- [TokTok#949] Fixes to the imported sodium sources to compile without warnings.
- [TokTok#948] Add a MAX_HOSTNAME_LENGTH constant.
- [TokTok#947] Remove the format test.
- [TokTok#937] Add new Circle CI configuration.
- [TokTok#935] Add a test for double conference invite.
- [TokTok#933] Add Logger to various net_crypto functions, and add `const` to Logger where possible.
- [TokTok#931] Avoid conditional-uninitialised warning for tcp test.
- [TokTok#930] Disable UDP when proxy is enabled.
- [TokTok#928] Use clang-format for C++ code.
- [TokTok#927] Add assertions to bootstrap tests for correct connection type.
- [TokTok#926] Make NULL options behave the same as default options.
- [TokTok#925] Add tests for what happens when passing an invalid proxy host.
- [TokTok#924] Make the net_crypto connection state an enum.
- [TokTok#922] Clarify/Improve test_some test
- [TokTok#921] Beginnings of a TCP_test.c overhaul
- [TokTok#920] Add test for creating multiple conferences in one tox.
- [TokTok#918] Merge irungentoo/master into toktok
- [TokTok#917] Add random testing program.
- [TokTok#916] Fix linking with address sanitizer.
- [TokTok#915] Remove resource_leak_test.
- [TokTok#914] Make dht_test more stable.
- [TokTok#913] Minor cleanup: return early on error condition.
- [TokTok#906] Sort bazel build file according to buildifier standard.
- [TokTok#905] In DEBUG mode, make toxcore crash on signed integer overflow.
- [TokTok#902] Log only the filename, not the full path in LOGGER.
- [TokTok#899] Fix macOS macro because of GNU Mach
- [TokTok#898] Fix enumeration of Crypto_Connection instances
- [TokTok#897] Fix ipport_isset: port 0 is not a valid port.
- [TokTok#894] Fix logging related crash in bootstrap node
- [TokTok#893] Fix bootstrap crashes, still
- [TokTok#892] Add empty logger to DHT bootstrap daemons.
- [TokTok#887] Fix FreeBSD build on Travis
- [TokTok#884] Fix the often call of event tox_friend_connection_status
- [TokTok#883] Make toxcore compile on BSD
- [TokTok#878] fix DHT_bootstrap key loading
- [TokTok#877] Add minitox to under "Other resources" section in the README
- [TokTok#875] Make bootstrap daemon use toxcore's version
- [TokTok#867] Improve network error reporting on Windows
- [TokTok#841] Only check full rtp offset if RTP_LARGE_FRAME is set
- [TokTok#823] Finish @Diadlo's network Family abstraction.
- [TokTok#822] Move system header includes from network.h to network.c
This pull request was closed.
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.

2 participants