Skip to content

Convert __main__ tests into unit tests (bis) - #540

Merged
asolino merged 10 commits into
fortra:python36from
infinnovation-dev:main2unit36
Dec 7, 2018
Merged

Convert __main__ tests into unit tests (bis)#540
asolino merged 10 commits into
fortra:python36from
infinnovation-dev:main2unit36

Conversation

@infinnovation-dev

Copy link
Copy Markdown

Reworking of #537 on python36 branch.

Several modules have main sections containing tests. These have been converted to proper unit tests. runall.sh has been updated accordingly.

Some of the original tests only printed results, so did not detect regressions when run via CI. In some cases (e.g. structure), there are additional checks on the binary form as opposed to just round-trip.

Note that test_dns currently fails for python2.7. Note that this is due to a regression (or at least a change in behaviour) between master and python36 branches: DNS.get_answers() now returns a domain name as unicode instead of str. I don't know how you want to deal with that.

@asolino

asolino commented Dec 6, 2018

Copy link
Copy Markdown
Collaborator

Thanks for redoing the work @infinnovation-dev.. much appreciated.

We will need to change the DNS tests, and probably others. Comparing the output text will generate these inconsistencies between different versions.

Not sure yet what the best approach is for this tho. I need to give it some thought.

@infinnovation-dev

Copy link
Copy Markdown
Author

Re DNS: from a python2.7 standpoint, the API has changed so that get_answers returns a unicode object for "Name" where before it was returning str (=bytes). From a bit of googling, it seems that the DNS protocol allows any octets in a label, so maybe it should be returned as bytes in python3 too. I don't think decoding as utf-8 is correct. On th other hand, normal well-behaved DNS servers will return ASCII names (IDNA punycode encoding is a higher-level consideration), so if a string result is preferred then maybe

    qrdata["Name"] = str(name.decode("ascii"))

That may result in an exception for non-ASCII labels, but then so might utf-8 decoding.

Alternatively, you could consider the encoding in RFC4343 section 2.1/2.2 but perhaps that's a step too far.

Whatever solution is chosen, I think should apply to MallExchanger too.

@asolino

asolino commented Dec 7, 2018

Copy link
Copy Markdown
Collaborator

Thanks for your answer @infinnovation-dev

I like the idea of

    qrdata["Name"] = str(name.decode("ascii"))

At least for now. I'll make that changes in dns.py

@asolino
asolino merged commit a43e434 into fortra:python36 Dec 7, 2018
@asolino

asolino commented Dec 7, 2018

Copy link
Copy Markdown
Collaborator

Merging. Thanks @infinnovation-dev!

@infinnovation-dev
infinnovation-dev deleted the main2unit36 branch December 7, 2018 17:58
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.

2 participants