search

rthalley / dnspython

a powerful DNS toolkit for python
http://www.dnspython.org
Other
2.45k stars 517 forks source link

DDR Test Anomolies #1137

Open kitterma opened 3 weeks ago

kitterma commented 3 weeks ago

Describe the bug As discussed in #1136, I've been having issues with the DDR tests. I've experimented further and have one finding. To gather more information, I modified tests/test_ddr.py::test_basic_ddr_sync to print out information instead of passing or failing based on the asserts and then raising an error at the end to get the stdout buffer. I get different results depending on which connection I'm on, but they almost always appear to fail. Generally one of the name servers will succeed ('1.1.1.1' or '8.8.8.8') but at least on these connections, it's rare for both to succeed.

Before the res.try_ddr line in the test, res.nameservers is always a list of string (e.g. ['1.1.1.1']). After try_ddr, it can be one of three things (depending on which connection I'm using):

A list of string (unchanged) A list of class 'dns.nameserver.DoTNameserver' A list of class class 'dns.nameserver.DoHNameserver'

Since both strings and dns.nameserver objects are allowed as nameservers, this is fine, but the post-try_ddr string causes the test to fail since it doesn't match the asserts. Looking at the try_ddr code, it looks like this is an expected possibility. Does a test failure here make sense? It seems like a flaky test since DDR may or may not produce an updated list.

Would it make more sense to only fail the test if both '1.1.1.1' and '8.8.8.8' fail?

To Reproduce Not sure if that's possible.

Context (please complete the following information):

rthalley commented 3 weeks ago

Those tests always pass for me, so I'm guessing something on the hotel network was dropping packets or doing some other bad thing. The "list of string (unchanged)" case, while possible in try_ddr()'s design, should never happen with 1.1.1.1 or 8.8.8.8 as they both implement the DDR protocol. The test is trying to say "if I try DDR for both of these servers, both should switch away from ordinary port 53 DNS". So I'm not inclined to fix the test that way. Now that we have a more capable "nanonameserver" that can do DNS-over-HTTPS and such, we might be able to test locally instead of live. I'd like to get rid of all of the live stuff over time!