Enable, and report test status

[GrayHatter]

This change is

[GrayHatter]

tests are fixed on travis

closes #9

@iphydf @neduard

[neduard]

Reviewed 1 of 3 files at r1, 2 of 2 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[nurupo]

I might be pointing out obvious, but Travis for this PR fails at tests.

[nurupo]

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

auto_tests/tox_test.c, line 943 [r2] (raw file):

        uint8_t dpk[TOX_PUBLIC_KEY_SIZE];
        tox_self_get_dht_id(toxes[0], dpk);
        TOX_ERR_BOOTSTRAP error = 0;

Why was this variable added? It's not used anywhere besides the call right following it. It's not even used in tox_bootstrap() two lines below it. Tthe other tox_add_tcp_relay()/tox_bootstrap() section you edited is not using error variable too, making the use of error values among tox_add_tcp_relay()/tox_bootstrap() calls quite inconsistent.

---

Comments from Reviewable

[GrayHatter]

That failure is due to group chats being fundamentally broken.

It'll randomly work if I push a new commit. I suggest we drop that test from the suite for the time being.

---

Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.

---

auto_tests/tox_test.c, line 943 [r2] (raw file):

Previously, nurupo wrote…

Why was this variable added? It's not used anywhere besides the call right following it. It's not even used in tox_bootstrap() two lines below it. Tthe other tox_add_tcp_relay()/tox_bootstrap() section you edited is not using error variable too, making the use of error values among tox_add_tcp_relay()/tox_bootstrap() calls quite inconsistent.

This is just the first one I happen to fix. Once I got the error off this one, I fixed the 2nd without adding this var. In talking with @neduard we discussed that these tests are likely to be replaced with something different. So Putting in the time now to fix them, didn't make too much sense.

Options are:

Rewrite this, and the next, whole session to be more descriptive and verbose. (Likely the best option, and will be done at some point if not done now.)

Quick fixup to make things more consistant.

Ignore it because tests currently "work", and we're likely to replace them after completing the spec tests anyways. (What I had planned to do.)

---

Comments from Reviewable

[iphydf]

Review status: 2 of 3 files reviewed at latest revision, 5 unresolved discussions.

---

.travis.yml, line 99 [r3] (raw file):

notifications:
  irc: "irc.freenode.net#toktok-status"

https://freenode.net/ says chat.freenode.net.

---

auto_tests/tox_test.c, line 5 [r3] (raw file):

 * Tox Tests
 *
 * These tests required that no other Tox clients are running/accessable at

"require_"
"accessible on_"

---

auto_tests/tox_test.c, line 6 [r3] (raw file):

 *
 * These tests required that no other Tox clients are running/accessable at
 * localhost. These test expect and the timeouts depend on the speed and size

"These tests*"
what do they expect?

---

auto_tests/tox_test.c, line 8 [r3] (raw file):

 * localhost. These test expect and the timeouts depend on the speed and size
 * of a private Tox network, trying to connect to outside clients will increase
 * the length of time each test will take. Often surpassing a reasonable timeout

Remove "length of ". Replace ", trying to" with ". Trying to" and replace ". Often" with ", often". Add "." at end of sentence.

---

Comments from Reviewable

[GrayHatter]

Review status: 2 of 3 files reviewed at latest revision, 5 unresolved discussions.

---

.travis.yml, line 99 [r3] (raw file):

Previously, iphydf wrote…

https://freenode.net/ says chat.freenode.net.

Done.

---

auto_tests/tox_test.c, line 5 [r3] (raw file):

Previously, iphydf wrote…

"require_"
"accessible on_"

Done.

---

auto_tests/tox_test.c, line 6 [r3] (raw file):

Previously, iphydf wrote…

"These tests*"
what do they expect?

Done.

---

auto_tests/tox_test.c, line 8 [r3] (raw file):

Previously, iphydf wrote…

Remove "length of ". Replace ", trying to" with ". Trying to" and replace ". Often" with ", often". Add "." at end of sentence.

Done.

Also can I have instructions each on their own line... That was very hard to read as a dyslexic

---

Comments from Reviewable

[sudden6]

auto_tests/tox_test.c, line 1241 [r4] (raw file):

        int num_peers = tox_group_number_peers(toxes[i], 0);

        /* We might need to run this test twice */

might add a reason why we need to run this twice

---

Comments from Reviewable

[GrayHatter]

Review status: 1 of 3 files reviewed at latest revision, 6 unresolved discussions, some commit checks failed.

---

auto_tests/tox_test.c, line 1241 [r4] (raw file):

Previously, sudden6 wrote…

might add a reason why we need to run this twice

The reason is that group chats are very broken, and split randomly. I don't think that should be the reason listed here. But I can add it.

---

Comments from Reviewable

[sudden6]

Review status: 1 of 3 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

auto_tests/tox_test.c, line 1241 [r4] (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

The reason is that group chats are very broken, and split randomly. I don't think that should be the reason listed here. But I can add it.

IMHO it wouldn't hurt to add ", because groupchats split randomly" or something like that

---

Comments from Reviewable

[GrayHatter]

Review status: 1 of 3 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

auto_tests/tox_test.c, line 1241 [r4] (raw file):

Previously, sudden6 wrote…

IMHO it wouldn't hurt to add ", because groupchats split randomly" or something like that

Will do

---

Comments from Reviewable

[neduard]

Reviewed 2 of 2 files at r4.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[iphydf]

Can you disable the tests that fail? So at least we'll have some tests running.

[GrayHatter]

Review status: 2 of 3 files reviewed at latest revision, 2 unresolved discussions.

---

auto_tests/tox_test.c, line 1241 [r4] (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

Will do

Done.

---

Comments from Reviewable

[iphydf]

Review status: 2 of 3 files reviewed at latest revision, 4 unresolved discussions.

---

auto_tests/tox_test.c, line 1241 [r4] (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

Done.

Done?

---

auto_tests/tox_test.c, line 6 [r5] (raw file):

 *
 * These tests require that no other Tox clients are running or accessible on
 * localhost. These tests expect, and the defined timeouts depend on the speed

What do the tests expect?

---

auto_tests/tox_test.c, line 36 [r5] (raw file):

/* The Travis-CI container responds poorly to ::1 as a localhost address
 * You're encouraged to -D FORCE_TESTS_IPV6 on a local test  */

Perhaps it's better to invert this logic, so a local test exercises more by default, and on travis we disable some things that don't work well there.

---

auto_tests/tox_test.c, line 1332 [r5] (raw file):

     * new version of group chats for Tox already completed, and nearly ready to
     * merge, No one is willing/available to give this test the time in needs */
    // DEFTESTCASE_SLOW(many_group, 140);

Use #if 0/#endif instead to accommodate for source formatters.

---

Comments from Reviewable

[GrayHatter]

Review status: 2 of 3 files reviewed at latest revision, 4 unresolved discussions.

---

auto_tests/tox_test.c, line 1241 [r4] (raw file):

Previously, iphydf wrote…

Done?

The done comment was to the one above, requesting a better comment as to why we had to run the test twice. I added that below, where I think it fits better.

---

auto_tests/tox_test.c, line 6 [r5] (raw file):

Previously, iphydf wrote…

What do the tests expect?

A small network size. If you connect to the global Tox Network, DHT traversing takes much longer than the timeouts are set for.

---

auto_tests/tox_test.c, line 36 [r5] (raw file):

Previously, iphydf wrote…

Perhaps it's better to invert this logic, so a local test exercises more by default, and on travis we disable some things that don't work well there.

I can do that, but is' just localhost. It's only Travis that breaks ::1 while 127.0.1 works. On other systems both should work exactly the same.

AFAICT there's no difference if you're system isn't ~broken.

---

auto_tests/tox_test.c, line 1332 [r5] (raw file):

Previously, iphydf wrote…

Use #if 0/#endif instead to accommodate for source formatters.

Done.

---

Comments from Reviewable

[nurupo]

Review status: 2 of 3 files reviewed at latest revision, 7 unresolved discussions.

---

auto_tests/tox_test.c, line 6 [r5] (raw file):

These tests expect, and the defined timeouts depend on the speed
I think it would be better written as "These tests expect -- and the defined timeouts depend on -- the speed". Had to re-read it at least twice due to the confusion caused by the change of tense in "expect" and "defined".

---

auto_tests/tox_test.c, line 36 [r5] (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

I can do that, but is' just localhost. It's only Travis that breaks ::1 while 127.0.1 works. On other systems both should work exactly the same.

AFAICT there's no difference if you're system isn't ~broken.

I'm for defaulting to 127.0.0.1 and enabling ::1 through a flag, as ipv4 is more commonly used by predating ipv6, thus it's less likely to break as everything has to support it.

---

auto_tests/tox_test.c, line 1244 [r5] (raw file):

        /* We might need to run this test twice */
        if (num_peers != NUM_GROUP_TOX) {
            ++test_run;

What is the terminating condition for repeating test runs? It seems like you will repeat the test until it passes, i.e. the number of reruns is unbounded, which is not nice if group-chats get broken by a code change and the test never finishes running because it always fails.

---

auto_tests/tox_test.c, line 1245 [r5] (raw file):

        if (num_peers != NUM_GROUP_TOX) {
            ++test_run;
            printf("\tError starting up the first group, going to restart this test. This is attempt %i\n");

You didn't pass an argument for %i to printf().

---

auto_tests/tox_test.c, line 1256 [r5] (raw file):

        }

        ck_assert_msg(num_peers == NUM_GROUP_TOX, "\n\tBad number of group peers (pre check)."

This is a useless statement, as the control flow will reach here only if num_peers == NUM_GROUP_TOX (it will take goto otherwise in the if statement above).

---

auto_tests/tox_test.c, line 1332 [r5] (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

Done.

Why did you work on improving `many_group` test just to disable it later? There are several issues with`many_group` test I have identified that you have to fix now.

---

Comments from Reviewable

[nurupo]

Reviewed 1 of 3 files at r1, 1 of 2 files at r4, 1 of 1 files at r5.
Review status: 2 of 3 files reviewed at latest revision, 7 unresolved discussions.

---

Comments from Reviewable

[nurupo]

Reviewed 1 of 1 files at r6.
Review status: all files reviewed at latest revision, 7 unresolved discussions.

---

Comments from Reviewable

[iphydf]

Review status: 2 of 3 files reviewed at latest revision, 8 unresolved discussions.

---

auto_tests/tox_test.c, line 36 [r5] (raw file):

Previously, nurupo wrote…

I'm for defaulting to 127.0.0.1 and enabling ::1 through a flag, as ipv4 is more commonly used by predating ipv6, thus it's less likely to break as everything has to support it.

I'd prefer it to break loudly in tests rather than eventually in production. If we can't test things reliably on travis, that's a pity, but on local tests we might catch things this way. The comment says "you're encouraged ...", so I'd suggest we make that the default. Chances are that $developer won't read this ever.

---

auto_tests/tox_test.c, line 1332 [r5] (raw file):

Previously, nurupo wrote…

Why did you work on improving many_group test just to disable it later? There are several issues withmany_group test I have identified that you have to fix now.

The work was done in an attempt to actually fix the test. That failed, so now we need to disable it, sadly. I hope we can enable it at some point, but right now we need _some_ tests at least.

Actually, perhaps we can turn this if 0 into an if TEST_GROUP_CHATS or something (maybe a better name?), enabled by default, disabled on travis?

---

auto_tests/tox_test.c, line 5 [r6] (raw file):

 * Tox Tests
 *
 * The following tests were written with a small Tox network in mind. Therefore,

80 characters?

---

Comments from Reviewable

[GrayHatter]

Review status: 2 of 3 files reviewed at latest revision, 8 unresolved discussions.

---

auto_tests/tox_test.c, line 6 [r5] (raw file):

Previously, nurupo wrote…

These tests expect, and the defined timeouts depend on the speed
I think it would be better written as "These tests expect -- and the defined timeouts depend on -- the speed". Had to re-read it at least twice due to the confusion caused by the change of tense in "expect" and "defined".

Done.

---

auto_tests/tox_test.c, line 36 [r5] (raw file):

Previously, iphydf wrote…

I'd prefer it to break loudly in tests rather than eventually in production. If we can't test things reliably on travis, that's a pity, but on local tests we might catch things this way. The comment says "you're encouraged ...", so I'd suggest we make that the default. Chances are that $developer won't read this ever.

@iphydf, I'm more with nurupo on this. With this comment, I'm only speaking to people who might read that comment. You as a xTox developer, can do what you'd like. Or as a distro package maintainer, also can do what ever. But if you're reading the source of the tests. I'd like you to try to run the test on IPv6. You're likely to catch a new bug before git-bisect get's huge.

---

auto_tests/tox_test.c, line 1244 [r5] (raw file):

Previously, nurupo wrote…

What is the terminating condition for repeating test runs? It seems like you will repeat the test until it passes, i.e. the number of reruns is unbounded, which is not nice if group-chats get broken by a code change and the test never finishes running because it always fails.

That's correct, because I did originally allow it to run twice. Then on the very next test, it split twice. So I ran it again, and it spilt 4 times in a row.

The timeout is the only thing that'll stop in inf loop. But this test is disabled in whole.

---

auto_tests/tox_test.c, line 1245 [r5] (raw file):

Previously, nurupo wrote…

You didn't pass an argument for %i to printf().

Done.

---

auto_tests/tox_test.c, line 1332 [r5] (raw file):

Previously, nurupo wrote…

Why did you work on improving many_group test just to disable it later? There are several issues withmany_group test I have identified that you have to fix now.

Because I thought it could be saved. But it's not the test that's broken, it's group chats themselves.

---

Comments from Reviewable

[GrayHatter]

Review status: 2 of 3 files reviewed at latest revision, 8 unresolved discussions, some commit checks failed.

---

auto_tests/tox_test.c, line 1332 [r5] (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

Because I thought it could be saved. But it's not the test that's broken, it's group chats themselves.

@iphydf, I could, but why. They're not going to suddenly work. So any changes to groups that fix them, is also likely to require the tests to be rewritten anyways.

---

auto_tests/tox_test.c, line 5 [r6] (raw file):

Previously, iphydf wrote…

80 characters?

it's before 80, as are most of the comments. Toxcore style says 120 is reasonable, but most of the source breaks that anyway.

---

Comments from Reviewable

[iphydf]

Review status: 2 of 3 files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.

---

auto_tests/tox_test.c, line 36 [r5] (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

@iphydf, I'm more with nurupo on this. With this comment, I'm only speaking to people who might read that comment. You as a xTox developer, can do what you'd like. Or as a distro package maintainer, also can do what ever. But if you're reading the source of the tests. I'd like you to try to run the test on IPv6. You're likely to catch a new bug before git-bisect get's huge.

Ok then. You got #22 for later.

---

auto_tests/tox_test.c, line 5 [r6] (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

it's before 80, as are most of the comments. Toxcore style says 120 is reasonable, but most of the source breaks that anyway.

Well. Ok then. I don't care that much, I just thought the line breaking in reviewable was ugly.

---

Comments from Reviewable

[nurupo]

Reviewed 1 of 1 files at r7.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

auto_tests/tox_test.c, line 1244 [r5] (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

That's correct, because I did originally allow it to run twice. Then on the very next test, it split twice. So I ran it again, and it spilt 4 times in a row.

The timeout is the only thing that'll stop in inf loop. But this test is disabled in whole.

Hm, then I'd like you to add a comment saying something along the lines "The test re-running forever (i.e. not having a stop condition) is not an issue because it will be terminated by the testing framework on a timeout".

---

Comments from Reviewable

[GrayHatter]

Review status: 1 of 3 files reviewed at latest revision, 3 unresolved discussions.

---

auto_tests/tox_test.c, line 1244 [r5] (raw file):

Previously, nurupo wrote…

Hm, then I'd like you to add a comment saying something along the lines "The test re-running forever (i.e. not having a stop condition) is not an issue because it will be terminated by the testing framework on a timeout".

Done.

---

auto_tests/tox_test.c, line 1256 [r5] (raw file):

Previously, nurupo wrote…

This is a useless statement, as the control flow will reach here only if num_peers == NUM_GROUP_TOX (it will take goto otherwise in the if statement above).

Done.

---

Comments from Reviewable

[GrayHatter]

Review status: 1 of 3 files reviewed at latest revision, 3 unresolved discussions.

---

auto_tests/tox_test.c, line 1241 [r4] (raw file):

Previously, GrayHatter (Gregory Mullen) wrote…

The done comment was to the one above, requesting a better comment as to why we had to run the test twice. I added that below, where I think it fits better.

Done.

---

Comments from Reviewable

[iphydf]

Waiting for @nurupo to review the requested changes.

---

Reviewed 1 of 3 files at r1, 2 of 2 files at r8.
Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

Comments from Reviewable

[iphydf]

Reviewed 1 of 1 files at r9.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[iphydf]

re-

---

Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

Comments from Reviewable

[iphydf]

Reassigning from @nurupo to @tux3 to accommodate for the former's urgent personal requirements

[tux3]

---

Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

Comments from Reviewable