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

coverage for injector_helper #44

Closed
shalunov opened this issue Jun 7, 2017 · 5 comments
Closed

coverage for injector_helper #44

shalunov opened this issue Jun 7, 2017 · 5 comments

Comments

@shalunov
Copy link
Contributor

shalunov commented Jun 7, 2017

To better understand what we'll need to do to get good test coverage, let's start with injector helper.

@shalunov
Copy link
Contributor Author

shalunov commented Jun 8, 2017

Is 58% the best we can have with integration tests? Does CI support IPv6? Why isn't the DHT joining code running? I think we could do a lot more.

@shalunov shalunov reopened this Jun 8, 2017
@inetic
Copy link
Contributor

inetic commented Jun 9, 2017

Currently the tests don't use DHT. Instead injector_helper knows about injector through a command line argument.

I did it this way because:

  1. The injector to which the injector_helper connects must be deterministic in tests.
  2. Given that the injector is only supposed to connect to the test injector_helper it very likely has not punched a hole through the NAT before the injector_helper attempts to connect.

(1) could potentially be solved by solving issue #13.

(2) is a bit trickier. I think Travis CI workers are not behind a NAT, not 100% sure though. In such case it would work there. For people behind a NAT though, the test would work only if they:

  1. Have a NAT with support for hairpinning. Or
  2. Explicitly open a UDP in their NAT. Or
  3. We use something like uPnP. Or
  4. We try to punch a hole ourselves.

I think I'd first try the fourth option...

Does CI support IPv6?

I'll check, but does libbtdht even supports IPv6? I was under the impression it doesn't (the string nodes6 is nowhere to be found in libbtdht as described by BEP0032, also the callback for finding nodes does not mentions type of the endpoints because only IPv4 is assumed).

@shalunov
Copy link
Contributor Author

There's some untested IPv6 code. Do we need it? If yes, let's cover; if not, let's remove?

@inetic
Copy link
Contributor

inetic commented Jun 13, 2017

There's some untested IPv6 code. Do we need it? If yes, let's cover; if not, let's remove?

AFAIK, that is really only three lines here and here (both are a copy/paste from here). We don't necessarily NEED it as it seems libbtdht doesn't support IPv6. On the other hand it's something we WANT, so the other course of action would be to add support for it in libbtdht.

But even if we did add it, I haven't yet checked whether Travis supports IPv6, so we still may not be able to cover it with tests.

@inetic
Copy link
Contributor

inetic commented Jun 14, 2017

Just to have a complete picture: It seems the OSX Travis workers do have a support for IPv6, while Linux workers don't.

OSX:

# ifconfig
lo0: flags=8049<UP,LOOPBACK,RUNNING,MULTICAST> mtu 16384
	options=1203<RXCSUM,TXCSUM,TXSTATUS,SW_TIMESTAMP>
	inet 127.0.0.1 netmask 0xff000000 
	inet6 ::1 prefixlen 128 
	inet6 fe80::1%lo0 prefixlen 64 scopeid 0x1 
	nd6 options=201<PERFORMNUD,DAD>
gif0: flags=8010<POINTOPOINT,MULTICAST> mtu 1280
stf0: flags=0<> mtu 1280
en0: flags=8863<UP,BROADCAST,SMART,RUNNING,SIMPLEX,MULTICAST> mtu 1500
	options=b<RXCSUM,TXCSUM,VLAN_HWTAGGING>
	ether 00:50:56:9a:09:1b 
	inet6 fe80::401:bc6b:ccc3:99e0%en0 prefixlen 64 secured scopeid 0x4 
	inet 10.182.152.84 netmask 0xffffc000 broadcast 10.182.191.255
	nd6 options=201<PERFORMNUD,DAD>
	media: autoselect (1000baseT <full-duplex>)
	status: active
utun0: flags=8051<UP,POINTOPOINT,RUNNING,MULTICAST> mtu 2000
	inet6 fe80::5c1f:7261:1086:5686%utun0 prefixlen 64 scopeid 0x5 
	nd6 options=201<PERFORMNUD,DAD>

Linux:

#ifconfig
eth0      Link encap:Ethernet  HWaddr 42:01:0a:0a:17:ca  
          inet addr:10.10.23.202  Bcast:10.10.23.202  Mask:255.255.255.255
          UP BROADCAST RUNNING MULTICAST  MTU:1460  Metric:1
          RX packets:11979 errors:0 dropped:0 overruns:0 frame:0
          TX packets:8618 errors:0 dropped:0 overruns:0 carrier:0
          collisions:0 txqueuelen:1000 
          RX bytes:190239647 (190.2 MB)  TX bytes:830041 (830.0 KB)
lo        Link encap:Local Loopback  
          inet addr:127.0.0.1  Mask:255.0.0.0
          UP LOOPBACK RUNNING  MTU:65536  Metric:1
          RX packets:32 errors:0 dropped:0 overruns:0 frame:0
          TX packets:32 errors:0 dropped:0 overruns:0 carrier:0
          collisions:0 txqueuelen:0 
          RX bytes:18041 (18.0 KB)  TX bytes:18041 (18.0 KB)

Adding a support for IPv6 however would seem like a task in itself. Here are the courses of action I can see, @shalunov please let me know which one you prefer:

  1. Leave the status quo (my preference for the MVP).
  2. Remove the 6 lines mentioning IPv6.
  3. Modify injector.c, injector_helper.c and test_server.c to use IPv6 when possible + leave libbtdht unchanged.
  4. As above, but also modify libbtdht to support IPv6.

ghazel pushed a commit that referenced this issue Jul 19, 2017
Coverage increase in injector_helper (resolves #44)
@shalunov shalunov closed this as completed Sep 5, 2017
ghazel added a commit that referenced this issue Dec 16, 2021
ghazel added a commit that referenced this issue Feb 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Injector helpers
In Progress
Development

No branches or pull requests

2 participants