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

fix(test): tests use ipv6 by default, even with USE_IPV6 set to 0 #2468

Merged
merged 2 commits into from
Jan 15, 2024

Conversation

Green-Sky
Copy link
Member

@Green-Sky Green-Sky commented Dec 13, 2023

fixes: #2335
It also fixes resolving to ipv6 addresses when its disabled.


This change is Reviewable

@emdee-is
Copy link

At the risk of doubling the size of your code, can I suggest "explicit is better than implicit" and adding the #else and opposite true case.

@emdee-is
Copy link

Looking good:

gdb auto_tests/auto_dht_getnodes_api_test
[Inferior 1 (process 1129058) exited normally]

ctest - most tests are passing except I'm getting a couple of timeouts: group_toxic and lan_discovery and tcp_relay and bootstrap @ 120 sec. - this could be because of tor. Is there an easy way of increasing the testing timeout? (as I said there are 2 timeouts I feel need increasing in the core to run with tor, although right now I'm running unmodified - I'll change them and retest.)

Finally - first ever reallife testing of tor ipv4!!!!!!!!!!!!!!!!!

@iphy - can we get a CI with a qemu build with ipv6.disable on the command line?

ctest3.log

@Green-Sky
Copy link
Member Author

Green-Sky commented Dec 14, 2023

At the risk of doubling the size of your code, can I suggest "explicit is better than implicit" and adding the #else and opposite true case.

nah, IF we do both sides, we better do

tox_options_set_ipv6_enabled(default_opts, USE_IPV6);

@emdee-is
Copy link

emdee-is commented Dec 14, 2023

Sure great idea - C handles 0/1 for true/false? Much better - ignore my C :-)

One less failure at 180 sec: 94% tests passed, 3 tests failed out of 54

Total Test time (real) = 751.85 sec

The following tests FAILED:
29 - lan_discovery (Timeout)
50 - bootstrap (Timeout)
51 - tcp_relay (Timeout)

I've seen group join take 20 minutes under tor - can you set a per-test timeout? I'll try 1200 next.

@emdee-is
Copy link

OK - we need a test for this with a container that has a linux kernel commandline.

@emdee-is
Copy link

@Green-Sky how do I tell what 2 tests are failing on a PR?

@nurupo
Copy link
Member

nurupo commented Dec 16, 2023

Is there an easy way of increasing the testing timeout?

@emdee-is TEST_TIMEOUT_SECONDS

@nurupo
Copy link
Member

nurupo commented Dec 16, 2023

how do I tell what 2 tests are failing on a PR?

Have you tried scrolling down the list of checks?

@Green-Sky
Copy link
Member Author

looks like this makes us observe issues described in #1026 and #1432

@Green-Sky
Copy link
Member Author

It would be cool if we could print the stack when we fire a "critical" in the ci.

@Green-Sky Green-Sky marked this pull request as ready for review December 16, 2023 10:54
@emdee-is
Copy link

(I hate autohiding scrollbars - thanks.)

#1432 used to drive me nuts - went into the code and turned it off, but not recently - or did I turn off toxcore logging in toxygen - maybe that was my answer!? I always compile DEBUG...

I raised #1432 in NGC a lot because I thought it shouldn't even be sending ipv6 packets when it was disabled - but got told SNAFU. I don't think I knew that there was an old issue open on it.

You're the first dev in history to try testing Tox ipv6.disable=1! I'll open a separate issue on adding a test to the testsuite.

I kept raising #1432

@emdee-is
Copy link

emdee-is commented Jan 9, 2024

Is there an easy way of increasing the testing timeout?

@emdee-is TEST_TIMEOUT_SECONDS

@nurupo Yes I saw that, but that's at compile time - I was hoping dor something at run time as there are only one or two tests I expect to run long. No problem - I'll set it long for them all.

Copy link

codecov bot commented Jan 14, 2024

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (29fc5ea) 71.16% compared to head (001d00a) 71.11%.

Files Patch % Lines
toxcore/network.c 0.00% 3 Missing ⚠️
toxcore/tox.c 50.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2468      +/-   ##
==========================================
- Coverage   71.16%   71.11%   -0.05%     
==========================================
  Files         127      127              
  Lines       28514    28517       +3     
==========================================
- Hits        20291    20280      -11     
- Misses       8223     8237      +14     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Green-Sky Green-Sky changed the title fix: tests use ipv6 by default, even with USE_IPV6 set to 0 fix(test): tests use ipv6 by default, even with USE_IPV6 set to 0 Jan 15, 2024
@Green-Sky Green-Sky marked this pull request as draft January 15, 2024 11:58
@Green-Sky Green-Sky marked this pull request as ready for review January 15, 2024 12:12
@Green-Sky Green-Sky force-pushed the fix_test_on_ipv4_only branch 2 times, most recently from 70db77f to 01e1825 Compare January 15, 2024 12:21
@Green-Sky Green-Sky merged commit 001d00a into TokTok:master Jan 15, 2024
54 of 55 checks passed
@Green-Sky
Copy link
Member Author

@emdee-is please try latest master again.

@iphydf iphydf modified the milestones: v0.2.20, v0.2.19 Jan 15, 2024
@emdee-is
Copy link

emdee-is commented Feb 1, 2024

@Green-Sky still getting some timeouts and one failure, but that's a huge improvement. #2335 (comment)

I think the failure in proxy test is expected if you are behind a firewall - it assumes open access to clearnet to run a proxy. (I'm wrong - the test sets up it's own network on localhost and never goes to the Internet.)

Please break out the ctest list of BSnodes #2467

@emdee-is
Copy link

emdee-is commented Feb 5, 2024

Signing this off as working - many thanks. I still get

The following tests FAILED:
         51 - bootstrap (Timeout)
         52 - tcp_relay (Timeout)
         56 - proxy (Failed)

but that's for other reasons: #2469

@Green-Sky
Copy link
Member Author

if you rerun bootstrap and tcp_relay with longer timeout, does it work?

@emdee-is
Copy link

emdee-is commented Feb 5, 2024

Nope: 3600 sec.

Expected - it doesn't know the proxy details. What surprised me is that
they are the only 2 ctests that use the network...

@Green-Sky Green-Sky deleted the fix_test_on_ipv4_only branch March 8, 2024 09:31
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.

ipv6 disabled on kernel cmdline disrupts Tox = most tests fail
4 participants