Testing policy - discussion

[pszal]

We should agree on our code testing policy. Current "de facto" rule is: every change to lib/ has to be covered by an unit test.

I don't believe it makes sense at the current stage: 1) dev time is ~2 times longer (I'd say at least 2 times...) 2) I don't see any good reason to spend time on unit tests for some inits, getters, setters, and other one-liners. We started using #pragma: no cover but we didn't specify when and where it is allowed (only @kormat knows that;-). 3) I don't see any good reason to write unit tests for intermediate PRs (i.e., WiP code) 4) from my point of view effectiveness of unit tests is so far really poor: a) we have >10k LOCs for unit tests, but I don't remember when last time it actually helped me to find or avoid a bug, b) for integration testing we use <300 LOCs (sic!) and this end2end test helped me to identify almost all bugs I encountered.

Very good and recent example is #462, which happened because there is no integration test for revocation at all.

My conclusion is that our code lacks of integration tests in the first place. Mandating unit tests for everything and ignoring integration tests does not make too much sense to me.

Please share your thoughts and experiences.

[pszal]

not too much discussion here;-), what about the following:

unit tests are mandatory for everything new added to lib/ with exceptions:

• not required for __str__, __repr__, __init__ (not sure), __len__ (not sure), getters, setters
• not required for WiP code. If we have problems with defining what is WiP code then we could use milestones on sub-project granularity (e.g. sub-projects like CS refactoring, revocation mechanisms, ZK support for X, etc... split into smaller tasks covered by PRs). And then e.g. the last, and mandatory step could be unit tests.

It is trickier with integration tests, but obviously every (major?) merged feature should be tested. scion.sh run and end2end cover a lot of stuff, and probably to test features like revocation we could just use ./supervisord.sh stop er... and assert that revocation message is received after few seconds. Can we use some framework here?

[aznair]

In my previous job, unit tests were mandatory but they were useless to the point of being a formality more than actual tests. Where stuff broke down is the integration tests, overnight aging tests, stress tests, etc. so we should definitely have more of those that cover more cases than end2end.

We also need a good way to run and verify these tests other than "dig through the logs" but that probably needs more time and discussion than we can have here.

[ercanucan]

Hi Pawel,

I think effective testing is very important, otherwise software is bound to break. The workflow I observed in SwiftKey was the following:

• Unit tests: Every function which does a non-trivial computation (say a function reversing a linked list or tokenizing a string) has to have unit tests with good set of inputs to cover the input spectrum. Trivial functions such as getters and setters are not tested because they are "assumed" to work as expected.
• Integration tests: Every new feature that is to be added to a software comes with its own important and characteristic tests. For instance, imagine a new API end-point which will be added to a server-side code. This end-point would work by receiving an HTTP request, fetching some data from a database, and returning the result to the client as a JSON file. For such a feature, there would be an integration test which would do the following:
  • Brings up a local database and populates it with test data.
  • Brings up a local instance of the server.
  • Brings up a local testing client.
  • Then the test client runs several times, builds HTTP requests with various possible parameters, sends those to the end-point and asserts that the json response object came exactly as expected (containing the expected fields, expected values inside etc.).

In our case for example, it may be a good idea to invest some effort in developing a testing framework which will let us write tests to create a "test topology", bring down or modify some links and observe a certain outcome. I think such tests can be very useful to have a stable core infrastructure code.

On Wed, Nov 11, 2015 at 12:15 PM, Jason Lee notifications@github.com wrote:

In my previous job, unit tests were mandatory but they were useless to the point of being a formality more than actual tests. Where stuff broke down is the integration tests, overnight aging tests, stress tests, etc. so we should definitely have more of those that cover more cases than end2end.

We also need a good way to run and verify these tests other than "dig through the logs" but that probably needs more time and discussion than we can have here.

— Reply to this email directly or view it on GitHub https://github.com/netsec-ethz/scion/issues/483#issuecomment-155738819.

[perrig]

The mininet environment that David and Stephen put together would be a great way to make network links fail in the tests ...

On Wed, Nov 11, 2015 at 1:46 PM, ercanucan notifications@github.com wrote:

Hi Pawel,

I think effective testing is very important, otherwise software is bound to break. The workflow I observed in SwiftKey was the following:

• Unit tests: Every function which does a non-trivial computation (say a function reversing a linked list or tokenizing a string) has to have unit tests with good set of inputs to cover the input spectrum. Trivial functions such as getters and setters are not tested because they are "assumed" to work as expected.
• Integration tests: Every new feature that is to be added to a software comes with its own important and characteristic tests. For instance, imagine a new API end-point which will be added to a server-side code. This end-point would work by receiving an HTTP request, fetching some data from a database, and returning the result to the client as a JSON file. For such a feature, there would be an integration test which would do the following:
• Brings up a local database and populates it with test data.
• Brings up a local instance of the server.
• Brings up a local testing client.
• Then the test client runs several times, builds HTTP requests with various possible parameters, sends those to the end-point and asserts that the json response object came exactly as expected (containing the expected fields, expected values inside etc.).

In our case for example, it may be a good idea to invest some effort in developing a testing framework which will let us write tests to create a "test topology", bring down or modify some links and observe a certain outcome. I think such tests can be very useful to have a stable core infrastructure code.

On Wed, Nov 11, 2015 at 12:15 PM, Jason Lee notifications@github.com wrote:

In my previous job, unit tests were mandatory but they were useless to the point of being a formality more than actual tests. Where stuff broke down is the integration tests, overnight aging tests, stress tests, etc. so we should definitely have more of those that cover more cases than end2end.

We also need a good way to run and verify these tests other than "dig through the logs" but that probably needs more time and discussion than we can have here.

— Reply to this email directly or view it on GitHub <https://github.com/netsec-ethz/scion/issues/483#issuecomment-155738819 .

— Reply to this email directly or view it on GitHub https://github.com/netsec-ethz/scion/issues/483#issuecomment-155767981.

[kormat]

Ok, here are my thoughts on the subject.

I disagree that dev time is ~2 times longer. Having decent lib/ unit test coverage made it faar easier and safer for me to do the major refactorings i did over the summer. Writing code with unit testing in mind makes the code much more readable, and hence less buggy. Writing unit tests for code makes you consider lots of different failure scenarios than you might otherwise do, which i find usually makes me improve the clarity/robustness of the code. Writing unit tests definitely takes time, yes, but debugging usually takes muuch longer if things aren't caught by tests.

I agree that very simple very straightforward methods don't need unit tests. Most __init__ functions fall into this category. This is what # pragma: no cover is intended for.

I'm probably ok with WIP code not having unit tests, provided i can be confident that it will get units soon after (soon being on the order of days, not weeks or months).

Integration tests are good, but cannot possibly exercise all code paths, not without an order of magnitude more test code than actual code. We definitely need more, they help a lot, but they do not replace unit tests (and vice-versa).

[kormat]

Every major feature should have integration tests. All code in lib/ should have unit tests, and we should aim to have unit tests for infrastucture/ and sciond.py too.

Having more/better integration tests is going to require some framework, agreed. That's going to be a bit of work, i have some vague ideas in mind, and it should be in the 0.2 milestone (at the latest).