Fix cname issue

[BiffoBear]

closes #19 #24

1. Wrote some unit tests to make sure that my changes don't change the external behaviour of the DNS library, nor it's interaction with the socket to send and receive data.
2. Refactored the code to separate reading the DNS record from parsing the DNS record.
3. Refactored building the query and parsing the response into functions so that they can be unit tested without mocking out the WIZNET5K interface and the socket.
4. Refactored the DNS query builder to hardcode most of it, as only the question name and query ID vary.
5. Refactored the DNS response parser to mask out flags that don't affect parsing, then check the remaining flags.
6. Refactored the DNS response parser to correctly parse answers until a type A, class IN one is found.
7. Future-proofed the DNS parser by raising exceptions, in case, perhaps, one day, the -1 s go away.
8. Refactored the gethostbyname function to handle the exceptions and maintain the current -1 ness.

[BiffoBear]

This fix has been tested on a Wiznet 5500 with 489 unique domain names. Without the fix, 192 did not return an IPv4 address. With the fix, 15 domain names did not resolve and none of these 15 responses included an IPv4 address.

[brentru]

@BiffoBear Could you resolve the branch conflict on adafruit_wiznet5k/adafruit_wiznet5k_dns.py and I can review?

Also include the REPL output from your hardware test,

Without the fix, 192 did not return an IPv4 address. With the fix, 15 domain names did not resolve and none of these 15 responses included an IPv4 address.

[BiffoBear]

Here are the REPLs from the old version, the new version, and the failures from the new version with debug = True to show that there are no Type A, class IN answers.
Bad_DNS_results.txt
New_DNS_results.txt
Old_DNS_results.txt

Not sure how I can resolve the merge conflict. My (admittedly limited) understanding of GIT is that this will require a manual merge? Please advise.

[BiffoBear]

@BiffoBear Could you resolve the branch conflict on adafruit_wiznet5k/adafruit_wiznet5k_dns.py and I can review?

Hi, I've merged the latest version into the PR and there are no more conflicts.

[FoamyGuy]

I merged main to this branch to resolve conflicts. I'll test this one out when I get back into this again in the upcoming week.

@BiffoBear do you happen to still have the code that ran the tests with the results posted above? I can try to whip up something to spot check a few, but if you've got it around still and can share it'd help make testing quicker for me.

[BiffoBear]

I merged main to this branch to resolve conflicts. I'll test this one out when I get back into this again in the upcoming week.

@BiffoBear do you happen to still have the code that ran the tests with the results posted above? I can try to whip up something to spot check a few, but if you've got it around still and can share it'd help make testing quicker for me.

@FoamyGuy Hi, I have the Python script, see below. Here are text files with the URLs that you can iterate through. One has URLs that resolve with the original version(URLs resolve with new and old.txt). The other has URLs that failed with the original (URLs only resolve with new.txt). Most of these now resolve with the new parser. There are a few that still fail because the response didn't contain an URL that we can use. The serial output from these few is in Bad_DNS_results.txt
Bad_DNS_results.txt
URLs only resolve with new.txt
URLs resolve with new and old.txt

[BiffoBear]

@FoamyGuy Cocktail coded a test script. YMMV

test dns code.py.txt

[FoamyGuy]

@BiffoBear thank you!

I tested this out successfully with the given code and lists of URLs using a Feather ESP32-S2 TFT and Ethernet Featherwing.

In my case the old version was always failing the second iteration with an exception raised about bytes object not having append. The first iteration did succeed or fail the same as noted by the file that I took it from.

With the new version I get vast majority successful responses from everything in both lists. I stopped it before it made it to the end, and there were a few no IP found sprinkled in, but way way more successful lookups than not.

I have one open question about one of the asserted values in a test, but everything looks good to me beyond that.

[FoamyGuy]

@BiffoBear I'm still getting the 21 vs. 22 error when I run the DNS test with the latest version:

test_dns_server_nonbreaking_changes.py:181 (TestDnsGetHostByName.test_retries_with_no_data_on_socket)
22 != 21

Expected :21
Actual   :22
<Click to see difference>

self = <test_dns_server_nonbreaking_changes.TestDnsGetHostByName object at 0x7f0229ab6d70>
wiznet = <MagicMock name='WIZNET5K' spec='WIZNET5K' id='139647258967488'>
wrench = <MagicMock name='socket' spec='socket' id='139647259624688'>

    def test_retries_with_no_data_on_socket(self, wiznet, wrench):
        """Confirm correct calls made to socket when no data available."""
        # Pylint does not understand that the wrench fixture is required.
        # pylint: disable=unused-argument
    
        dns_server = wiz_dns.DNS(wiznet, "8.8.8.8", debug=DEFAULT_DEBUG_ON)
        dns_server._sock.available.return_value = 0
        dns_server._sock.recv.return_value = b""
        dns_server.gethostbyname(bytes("domain.name", "utf-8"))
    
        # Check how many times the socket was polled for data before giving up.
        dns_server._sock.available.assert_called()
>       assert len(dns_server._sock.available.call_args_list) == 21
E       AssertionError: assert 22 == 21
E        +  where 22 = len([call(),\n call(),\n call(),\n call(),\n call(),\n call(),\n call(),\n call(),\n call(),\n call(),\n call(),\n call(),\n call(),\n call(),\n call(),\n call(),\n call(),\n call(),\n call(),\n call(),\n call(),\n call()])
E        +    where [call(),\n call(),\n call(),\n call(),\n call(),\n call(),\n call(),\n call(),\n call(),\n call(),\n call(),\n call(),\n call(),\n call(),\n call(),\n call(),\n call(),\n call(),\n call(),\n call(),\n call(),\n call()] = <MagicMock name='socket().available' spec='function' id='139647259705248'>.call_args_list
E        +      where <MagicMock name='socket().available' spec='function' id='139647259705248'> = <NonCallableMagicMock name='socket()' spec='socket' id='139647259634240'>.available
E        +        where <NonCallableMagicMock name='socket()' spec='socket' id='139647259634240'> = <adafruit_wiznet5k.adafruit_wiznet5k_dns.DNS object at 0x7f022931fc10>._sock

test_dns_server_nonbreaking_changes.py:194: AssertionError

[BiffoBear]

@FoamyGuy I've added a @freezegun decorator to the test. Hopefully, that will give the same number of calls across any platform that the tests are run on. With Freezegun I get 12 on mine (I was getting 21 calls when the test ran on the system clock, so I guess that yours is running a little faster than mine!).

Please let me know whether you get 12 as well.

[FoamyGuy]

This looks good to me now. Thank you @BiffoBear for these improvements and sticking with it through some changes!

The latest version does allow the tests to pass for me locally with a value of 12 for the one that was problematic before.

I tested these changes successfully on Feather TFT S2.