Testravaganza2: Improve test coverage to 98%

crocogorical commented 3 years ago

Added one or two more tests.

Have kept the code predominantly to the testing code, but there are a few minor modifications to the processing logic where it was internally inconsistent (and invalid python).

I have also added in the parseopts argument option to tox.ini. This compensates for the removal of the legacy test runners by allowing running a single module or test at a time. Example use: tox -e py39 -- dpkt/netbios.py::test_rr.

Some minimal notes on various files which are not specifically test related, but seem a little too small to create specific issues for. Can break them out into separate PRs?

netbios.py

NS.pack_name and NS.unpack_name reference dpkt.dns.DNS methods which do not exist (and have FIXME comments indicating this). Since these can never have worked, they should be removed.

tftp.py

TFTP.errcode was a struct unpack with a single element, but the re-packing expected a single int. Unpacking now explicitly takes the first element.

edp.py

__str__ in the case of a zero .sum attribute attempts to recalculate the checksum. This, in turn calls dpkt.in_cksum, buf this will fail as it requires bytes, not str. Replaced __str__ with __bytes__ method.

telnet.py

Line 66 cannot be reached. The only int in the range [0, 255], received from indexing a bytes object is 0, and line 64 replaces all b'\x00's with b'\n'. Lines 65/66 should be removed.

ip6.py

IP6.__bytes__ has try/except around self.data.sum., but it is not possible to reach that line without passing through a conditional involving self.data.sum. It seems impossible to raise this exception, so should be removed.

pcap.py

DLT_LOOP and DLT_RAW are both redefined conditionally based on the current system type being openbsd or not. Remove initial definition?
setfilter method returned NotImplementedError, rather than raising it. It now raises it.

asn1.py

utctime catches a TypeError with int(buf[10:12]). Invalid characters in this position will raise a ValueError. The only way to raise a TypeError is to pass the wrong type, which is impossible? Having parsed int() on this same buffer already.

h225.py

H225.IE.__bytes__ attempts to concat None to bytes if self.type & 0x80, which fails.

gre.py

__bytes__ had b''.join(fmts) on line 117, but this will never work as fmts is a list of strs. Removed the b.

brifordwylie commented 3 years ago

@crocogorical thanks a bunch for the change/increased test coverage. As you know, the crew here is skeleton, and can be hit-and-miss on reviews. Although in general the code changes look good, I think smaller PRs will be reviewed/go in quicker. This PR is fine.. but maybe something to keep in mind for future PRs :)

Also, the test coverage is good/important, but I have mixed feelings on taking out the 'main stubs'

if __name__ == '__main__':  
    test_aoeata()   
    print('Tests Successful...')

I get that this will reduce the number of untested lines, but it also takes away the simplicity of changing the code and running it from the command line or inside an IDE (like PyCharm).. change some code, set a breakpoint, and hit the green arrow/bug :)

Yes, you can run the code with Pytest (on both the command line and PyCharm) but not everyone is familiar with how to do that (although your tox.ini changes helps :) .. shrug... I guess I don't feel strongly either way... @obormot @kbandla any thoughts on this.. to be honest I'm fine with removing the 'main test stubs' if other folks would like that.

crocogorical commented 3 years ago

I understand the desire for sinple means to run tests during development, having done a bit of it recently.

The existing method is:

$ python dpkt/ethernet.py

Compared to running through tox which would be:

$ tox -e py38 dpkt/ethernet.py

Rather than type it all out, I found that using something like when-changed was an excellent timesaver.

$ when-changed -s -r dpkt -c 'tox -e py38 $WHEN_CHANGED_FILE'

Whenever a file is changed in the recursively monitored directory, tox is immediately run on that file.

All of this works seamlessly with breakpoints too. Once you continue from the breakpoint, the test continues, just as if you had run the file directly.

A benefit to using tox/pytest is that when the assertion fails, it does some of the next steps for you which is printing out what you were trying to compare, and highlighting where they differ. It's not perfect, but it's the next few steps that I would type out in the debugger to understand what was going wrong.

Perhaps a halfway house might be to move all test code into a tests/ directory, and then only run coverage on the dpkt/ directory? It would allow clear separation, remove stuff from the project namespace, and allow module-wise imports to assist in testing, rather than importing once per function.

If the projects acceptance criteria for PRs is passing tests, run by tox, then it makes sense for devs to be able to replicate that locally prior to opening a PR? I'm not sure what the preferred flow is.

On the size of the PR, I had considered separating then out by file, but thought 50 PRs might be a bit excessive.

brifordwylie commented 3 years ago

@crocogorical those are all good points. The PR looks good I'm going to go ahead and merge it, thanks again for your contributions.

kbandla / dpkt