Switching to test driven programming

[neslihanturan]

Summary:

Hi team, since our time spent for bug fixes seems a little high, maybe switching to test driven programming can be a solution. Since on our WMCZ grant session, we will have several quality improvement, then our code base may be ready for test driven programming. What do you think?

[misaochan]

I am assuming you meant "test-driven programming", so I took the liberty of editing your post. ;)

I agree, I think this would be well worth the investment. Would love comments from experienced devs especially - @nicolas-raoul @whym @dbrant @psh ?

[nicolas-raoul]

I don't think test-driven programming is a great fit for most parts of our project because:

• Most bugs are in parts that unit tests can not reach (or at least can not reach without a really big effort), such as the GUI or 2FA.
• In contrast with most software (most software is developed for companies, for instance a company's billing system or a factory's machine software), we have the chance to have a community of testers. Test-driven is a great way to reduce the number of bugs in the output of your IDE, but it comes at a great cost. Companies are more willing to afford it, because faulty software could result in destroying a factory machine, and they have no community to get feedback from.

That being said, there are parts of the project that could certainly benefit from test-driven programming, so I suggest starting with those. For instance the non-GUI part of EXIF parsing and EXIF modification, this could be a good start.

[neslihanturan]

If we can well define where we want to have it, we can include this as a rule to contributors documents. So that new comers also can now, they need to write tests if possible.

And since we can't write tests for GUI, maybe we can make sure we separated GUI layer as much as we can.

[whym]

In general, I think it's a good exercise to ask yourself 'can I write tests?' when creating and modifying a feature, and perhaps more importantly, when fixing a bug so that the same bug will not happen again easily.

We have utility classes and methods (e.g. app/src/main/java/fr/free/nrw/commons/Utils.java) for which we can naturally write tests. Perhaps we can start from those.

GUI features are harder to test, but not impossible in my opinion. I believe something like app/src/androidTest/java/fr/free/nrw/commons/SettingsActivityTest.java is a good way to catch bugs that might sneak in unintentionally. High coverage would probably be unrealistic, but a few GUI tests will be great to have to prevent breaking things.

[ilgazer]

In my opinion, having more tests could be beneficial. On the other hand, making tests a requirement could dissuade less experienced developers(that's me) from developing new features. For example, even though I built upon psh's MVP architecture for #1796, I would have shied away from working on that if I had to write tests for my code, something I have no experience or knowledge about. The same holds for my EXIF modification code. In conclusion, I think that while encouraging tests is a good idea, mandatory tests would do more harm than good.

[albendz]

I'm new to reviewing code for this project and I'm still trying to understand the quality bar for reviews. This recent example shows something I'm confused about: https://github.com/commons-app/apps-android-commons/pull/1888

Test driven development can be heavy handed, especially when new programmers in a formal program or learning on their own do not learn testing typically. However, it looks like we could make some small changes to the contributing guidelines to explicitly say that everything needs to be unit tested and if you can't do so easily, provide a reason in the PR and we will waive the need to unit test if it's UI or 2FA as Nicholas points out.

@ilgazer I agree that mandatory tests will be frustrating and potentially discourage new contributors. Would you think if we say "tests or a justification of why there are no tests is mandatory" would be better? Then the more experienced project test contributors can provide pointers.

[ashishkumar468]

Although TDD enables refactoring in an anxiety-free, predictable way, and of-course enforces standards, reaching slowly towards bug free and more stabler product, not all of the developers(actually most of them) follow it and actually know it. Even though it has a lot of advantages, we all know that it would require extra efforts to those who are not acquainted with it. Having it would be great, but might discourage early stage contributors. And if we talk about improvements in code standards improvement, of-course TDD would have been the best solution, but for the time period, until the app reaches stable in terms of development and adding new features (which i am not sure it is now), where we have enough of resources and time and not loaded with new features, IMO, and I may be wrong, we can go ahead with the current policy of not mandating it (but of-course encouraging it to those who can and are willing to go for it). Also code standards can and is being currently enforced by the code reviewers (thanks to the concept of PR's). But not accepting PR's because of test cases, in my opinion, would not be the best thing to do as of now. However I would recommend to introduce a code architecture pattern, say MVP or MVVM (Please correct me if we already follow any of these). That will automatically raise code standards and improve code quality.

[misaochan]

Thanks for the input everyone!

Based on the opinions here, my proposed solution:

• We make unit tests mandatory ONLY for grant tasks starting March 2019 (specifically, grant tasks where it would make sense to write tests, not UI etc tasks). This would certainly increase the amount of time needed for the tasks, so I would need to take a few items out of #1889 to make room for that. However, I personally believe that this is a good way of futureproofing our core functionality, especially given our open source nature (for instance, if a volunteer submits a seemingly-unrelated PR that breaks, say, category suggestions, Travis will tell us without the reviewer needing to manually test EVERYthing), so I feel that it would be worth the increased time cost.

• Keep unit tests as "optional" for volunteer PRs, albeit with encouragement to write them. However, once we have implemented mandatory unit tests for grant tasks for several months, we can reevaluate this, based on the state of the codebase at that time and whether or not we think the benefits are worth the additional entry barrier.

• If we do this, we should probably add a metric to #1889 for code coverage to aim for. However, I'm not sure what a realistic % would be, especially bearing in mind that new PRs from volunteers would likely not include coverage. I do get that code coverage isn't always an ideal standard, but if we want to justify taking 10-20% longer for our grant tasks, we will likely need to measure this (and after all, that SOF link does mention "satisfying stakeholders" as a benefit ;)).

What do you guys think?

[ilgazer]

@misaochan Your compromise of only requiring grantees to write unit tests is a really good idea. If we will apply as a GSoC project, maybe we could have the same requirement for them.

[misaochan]

@ilgazer Good idea! 👍

[ilgazer]

Also, I think linking to some good resources in the wiki about where to learn testing would help a lot. I distinctly remember being overwhelmed by the abundance of stuff to learn and giving up when I tried to start writing tests for #1712. I still have no clue about how to write tests.

[NotSoImportant]

Is this a stale issue?

[neslihanturan]

I don't think this is gonna happen soon. If you have suggestions/things to say about this issue feel free to share.

[NotSoImportant]

I was just wondering as something was put to done if now you have applied it or what are the next steps.

[misaochan]

I would consider this a semi-success. All PRs for new features/enhancements now require unit tests, as mentioned in our contribution guidelines, and our code coverage has also increased substantially. However, I don't think most contributors are applying true TDD principles (which is different from just writing tests for their code).