search

bitkeks / python-netflow-v9-softflowd

PyPI "netflow" package. NetFlow v9 parser, collector and analyzer implemented in Python 3. Developed and tested with softflowd
https://bitkeks.eu/blog/2016/08/collecting-netflow-v9-on-openwrt.html
MIT License
116 stars 59 forks source link

Fixing tests #18

Closed grafolean closed 3 years ago

grafolean commented 4 years ago

Hi @bitkeks!

First of all, thank you for this app, it works like a charm!

I am trying to do some performance optimizations on packet parsing (would be happy to create a PR later if you are interested and if I'm successful of course).

However while trying to fix this, I also tried running the tests.py to make sure I will be getting the same results, but stumbled across some trouble as 5 out of 6 tests failed to run, because tests were using the wrong index (p[1] instead of p[2]) when addressing the records. I assume client was added between timestamp and records somewhere along the line. I took the liberty of creating a PR for this.

Just a side note: test_analyzer still fails on my machine (Python 3.6) because it uses capture_output parameter to subprocess.run(), which was added in Python 3.7. But I assume that is not a problem for most users, and it doesn't affect me since I don't use the analyzer.

While on the subject, IIUC the decoded packets are not compared to some known representation in the tests? I am asking because I would like to avoid regressions when doing performance optimizations. Does adding this to the tests sound like a good idea to you, or does something like this even maybe already exist?

bitkeks commented 4 years ago

Hi @grafolean, sorry for the delay and thank you for your issue and PR! I have improved the handling of the queue items with a new namedtuple, ParsedPacket. Please see commit 0d8f1a2ecb6af9d46df70f2a87844292fd146a42 for details. It also removes the need for using indexes.

Just a side note: test_analyzer still fails on my machine (Python 3.6) because it uses capture_output parameter to subprocess.run(), which was added in Python 3.7. But I assume that is not a problem for most users, and it doesn't affect me since I don't use the analyzer.

That's good to know, didn't catch that. There's a backwards compatibility fix in the same commit.

While on the subject, IIUC the decoded packets are not compared to some known representation in the tests?

I'm not sure what you mean. The tests work by taking a bunch of example packets (binary encoded in PACKETS and TEMPLATE_PACKET) and sending them to a running collector, which then decodes the received binary packets into the NetFlow structure. Lastly, there are some checks applied to the parsed NetFlow export payload, e.g. self.assertEqual(len(pkts), num) for checking that the number of sent packets is the same as the decoded amount.

Does adding this to the tests sound like a good idea to you

Nevertheless, yes. Since this whole project is a playground for experiments, feel free to add new features! Especially if you are looking into performance improvements :smile: