QEP 30: Required unit testing for "critical" classes

[NathanW2]

QGIS Enhancement 30: Required unit testing for "critical" classes

Date 2015/10/20
Author Nyall Dawson
Contact nyall dot dawson at gmail dot com
Last Edited 2015/10/20
Status Accepted

Summary

This QEP describes a system of classifying certain classes within the QGIS codebase as critical
classes and proposes that no changes be allowed to these classes without accompanying unit
tests.

While QGIS contributor guidelines state that all changes to code within CORE be accompanied
by unit tests, this requirement is universally ignored. However, since the introduction of CI
testing of every commit and pull request to master the value of writing tests to prevent
regressions has tremendously increased. It is acknowledged that introduction of unit tests
does not automatically fix bugs, but it does help drastically reduce the potential of regressions.

This QEP describes a middle-ground approach to requiring compulsory unit tests for certain areas
of the codebase without placing too much additional burden on developers.

It is based on a well-defined minimal required test suite, and does not prevent
developers from writing additional tests or requiring tests for classes not
covered by this QEP.

Proposal

Any change to a class which is classified as "CRITICAL" MUST be accompanied by
unit testing to prevent regressions.

Pull requests which touch upon CRITICAL classes and which do not have unit
tests must NOT be merged to master. Commits pushed to master which
also violate this may be reverted if no suitable follow-up unit tests are added following discussion/consultation with the original committer.

Definition of CRITICAL classes

For a class to be classified as a CRITICAL class it must satisfy two requirements:

The class must relate to code which has:

• Potential for data loss for users
• Risk of incorrect calculations resulting in invalid analysis (eg, incorrect area
  or distance calculations)

This excludes code which relates to purely cosmetic components, eg symbology,
labeling, conditional styles, etc. The requirement is intended to identify areas
where regressions would result in significant harm to the reputation of QGIS
or potential risk of incorrect analysis products generated by QGIS and the wider
risks this entails.

The class must have 100% unit test coverage (or as close as possible)

This requirement is intended to reduce the workload on developers. While it is
relatively straightforward to adopt existing unit tests to cover new code, it
can be quite burdensome or complex to start creation of unit tests from scratch.
Requiring all changes to classes which satisfy 2.1.1 to have unit tests would likely
push away potential contributors who do not have the time or skill required
to write unit tests from scratch.

Accordingly, a class can only be categorised as CRITICAL when it already has
100% existing unit test coverage.

Proposed Technical Solution

When a class is categorised as CRITICAL, comments will be added to the source
code to warn developers that all changes must be accompanied by unit tests.
These would take the form of the line:

// CRITICAL code - changes must be accompanied with unit tests in testqgsfield.cpp

placed within the header, and between each function definition in the class cpp file.

The contributor guidelines would be updated to reflect this requirement.

For a class to be categorised as CRITICAL, a pull request will be opened which
includes the CRITICAL comment blocks for the class. The PR can then be used
for discussion regarding whether or not the class satisfies the requirements
for a critical class, specifically whether it satisfies 2.1.1.

Implementation Details

Initially, only the QgsField, QgsFields and QgsFeature classes would satisfy
the requirements for CRITICAL classes. It is the intention that the following
classes be prioritised to bring them up to CRITICAL status ASAP:

• all geometry classes
• QgsExpression
• QgsDistanceArea
• QgsStatisticalSummary

Voting History

+1

Andreas Neumann
Anita Graser
Otto Dassau

0
Paolo Cavallini
Richard Duivenvoorde
Marco Hugentobler

Other comments:

Andreas Neumann
If it helps to maintain/raise quality - I will support this QEP. Devs should be supportive, if a PR concerning core classes misses unit tests and the dev is not so familiar with unit tests. At least in the transition phase.

Jürgen E. Fischer
Generally I’m in favor of this. But the passage “and commits pushed to master which also violate this will be reverted.” is a nonono for me. Missing tests could probably be easily added with another commit - and reverting the commit in question IMHO isn’t necessary.
I think that this will probably (/hopefully) never happen in practice anyway and resolved by giving a dev in question a little nudge instead. But this is all about strict rules, so we would avoid rules that we don’t actually follow.

[nyalldawson]

@NathanW2 This is good to go, the issues raised from the previous PR have been incorporated too.

Can we move this to voting now?

[NathanW2]

Yep. Do you want to send something to the PSC for ms

On Sun, 1 Nov 2015 7:41 am Nyall Dawson [email protected] wrote:

@NathanW2 https://github.com/NathanW2 This is good to go, the issues
raised from the previous PR have been incorporated too.

Can we move this to voting now?

—
Reply to this email directly or view it on GitHub
#30 (comment)
.

[jef-n]

I'm against generally reverting a commit that doesn't have unit tests. That should be decided individually with common sense or by adding the missing unit tests.

[nyalldawson]

@jef-n I agree in principal, but I think it's important to have the potential consequence of violating this qep included as a worse-case scenario. Obviously a better solution is to communicate with the dev and negiotiate adding the required tests (with the background threat of reverting the commit as a last resort).

[jef-n]

Well, that's at least not what the QEP says. But I still think that the threat is unnecessary anyway.

[nyalldawson]

Except without the threat this QEP is effectively meaninglessness. It would mean that there's no change to the current situation... Ppl can modify these classes and it'll fall back to someone else to add tests.

[NathanW2]

If the class is critical then I see enforced rules good

On Sun, 1 Nov 2015 11:10 am Nyall Dawson [email protected] wrote:

Except without the threat this QEP is effectively meaninglessness. It
would mean that there's no change to the current situation... Ppl can
modify these classes and it'll fall back to someone else to add tests.

—
Reply to this email directly or view it on GitHub
#30 (comment)
.

[jef-n]

On Sat, 31. Oct 2015 at 18:10:02 -0700, Nyall Dawson wrote:

Except without the threat this QEP is effectively meaninglessness. It would
mean that there's no change to the current situation... Ppl can modify these
classes and it'll fall back to someone else to add tests.

No, it's not. But it's says that commits "will be reverted" - it's not the
ultimate option - it's the only one and it requires it.

A good commit shouldn't be reverted. And I don't think that every commit that
reduces test coverage is bad.

[NathanW2]

So what is the plan. IMO if something is marked as must have tests and there isn't any or indication that they will be added like @nyalldawson said it will just land on someone else to do them. Something like adding a bunch of functions to QgsExpression without tests is a good example.

[nyalldawson]

Note that there's additional discussion surrounding this on the old PR - #23

[anitagraser]

+1

[timlinux]

So I have some concerns about the overhead needed to manage this. For me a much simpler approach is to have a rule in place that says "Any PR merge should not reduce the coverage of our test suite." I think we don't want a class that has 50% coverage to be reduced any more than we want one that is 100% covered. Instead we should be generally aiming to increase our test coverage with each PR or at a minimum maintain the level of coverage.

If a committer has a particular reason why there PR does not maintain the coverage levels, the should explain why this is the case in their PR. There are some valid cases why coverage may drop - for example there may be a code branch that only gets executed in particular edge cases (e.g. on a specific platform). The PR reviewer should then review these justifications and decide if they constitute a blocker or not.

[timlinux]

Note a loomio vote has been started for this QEP here:

https://www.loomio.org/d/sW6H8ZpV/qep-17-required-unit-testing-for-critical-classes

[NathanW2]

I think the main goal of this when I read it is to stop things like:

// new best function to calculate the area - totally use this!
// NOTE: Didn't bother with unit tests use at your own risk might break but
we don't know.

etc

If something is critical I don't see a reason for never having test. We
can ifdef in unit tests to handle the platform cases.

On Thu, Nov 5, 2015 at 2:41 PM, Tim Sutton [email protected] wrote:

Note a loomio vote has been started for this QEP here:

https://www.loomio.org/d/sW6H8ZpV/qep-17-required-unit-testing-for-critical-classes

—
Reply to this email directly or view it on GitHub
#30 (comment)
.

[nyalldawson]

Also, important to keep in mind some things:

• realistically, we'll never have 100% test coverage by lines of code. It's just too much overhead/work, and too complicated for heavily gui based classes. It would also likely prevent people submitting fixes were a 5 minute bug fix becomes an hour long struggle to cover the fix by a unit test. We want to avoid that.
• But for SOME parts (those which relate to the criteria in this QEP) we SHOULDN'T settle for anything less than 100% coverage by LOC. A bug in symbology or digitizing tools may be frustrating for users, but ultimately it's harmless. But a bug in area calculation or expression results could potentially cost $$$/someone's job/maybe even put people at risk. That's what this QEP covers. Really deep requirement for test coverage for this critical stuff.

[wonder-sk]

Re-reading the proposal, I am not sure how much practical impact it may have... for the "simple" classes like QgsFeature, QgsFields and few others it is not so difficult to reach high test coverage and thus "critical" status. But I think we have long way to being able to reach high unit test coverage for some other classes that really matter - such as QgsVectorLayer - and which due to their complexity can barely ever get "critical" status.

Few more concerns:

• I think that it is rarely practical to go for 100% test coverage (again, except for the "simple" classes). Last few percent are typically very difficult to reach as they require setting up of lots of rare use cases and the maintenance of unit tests becomes a problem.
• Focusing on measurable things like code coverage is easy - but it is also easy to write tests that just bump up the coverage but are mostly useless otherwise.
• More important than the coverage of unit tests is their quality - how thoroughly do they test things, whether they are robust and not prone to false positives.

Rather than enforcing some strict rules I would prefer something simple - maybe what Tim proposes would be just fine (like it already works with docstring coverage).

Ideally together with some small peer pressure to devs that are lazy and skip writing tests to new core functionality (I know I'm sometimes guilty too).

[nyalldawson]

"commits which also violate this will be reverted." -> "Commits pushed to master which
also violate this may be reverted if after discussion/consultation with the original committer no suitable follow-up unit tests are added. "

@timlinux how's that sound?

[NathanW2]

That sounds a bit nicer to me as it gives someone time to do it if they
forget

On Mon, Nov 9, 2015 at 7:20 PM, Nyall Dawson [email protected]
wrote:

"commits which also violate this will be reverted." -> "Commits pushed to
master which
also violate this may be reverted if after discussion/consultation with
the original committer no suitable follow-up unit tests are added. "

@timlinux https://github.com/timlinux how's that sound?

—
Reply to this email directly or view it on GitHub
#30 (comment)
.

[timlinux]

@nyalldawson your amendment sounds good to me!

[nyalldawson]

Implemented in qgis/QGIS@ee44bc8 for:

• QgsField
• QgsFields
• QgsFeature
• QgsWKBTypes
• QgsStatisticalSummary