{"action":"created","issue":{"url":"https://api.github.com/repos/qgis/QGIS-Enhancement-Proposals/issues/30","labels_url":"https://api.github.com/repos/qgis/QGIS-Enhancement-Proposals/issues/30/labels{/name}","comments_url":"https://api.github.com/repos/qgis/QGIS-Enhancement-Proposals/issues/30/comments","events_url":"https://api.github.com/repos/qgis/QGIS-Enhancement-Proposals/issues/30/events","html_url":"https://github.com/qgis/QGIS-Enhancement-Proposals/issues/30","id":113788130,"number":30,"title":"QEP 17: Required unit testing for \"critical\" classes","user":{"login":"NathanW2","id":381660,"avatar_url":"https://avatars.githubusercontent.com/u/381660?v=3","gravatar_id":"","url":"https://api.github.com/users/NathanW2","html_url":"https://github.com/NathanW2","followers_url":"https://api.github.com/users/NathanW2/followers","following_url":"https://api.github.com/users/NathanW2/following{/other_user}","gists_url":"https://api.github.com/users/NathanW2/gists{/gist_id}","starred_url":"https://api.github.com/users/NathanW2/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/NathanW2/subscriptions","organizations_url":"https://api.github.com/users/NathanW2/orgs","repos_url":"https://api.github.com/users/NathanW2/repos","events_url":"https://api.github.com/users/NathanW2/events{/privacy}","received_events_url":"https://api.github.com/users/NathanW2/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/qgis/QGIS-Enhancement-Proposals/labels/Type/Project","name":"Type/Project","color":"fbca04"},{"url":"https://api.github.com/repos/qgis/QGIS-Enhancement-Proposals/labels/Voting","name":"Voting","color":"5319e7"}],"state":"open","locked":false,"assignee":{"login":"nyalldawson","id":1829991,"avatar_url":"https://avatars.githubusercontent.com/u/1829991?v=3","gravatar_id":"","url":"https://api.github.com/users/nyalldawson","html_url":"https://github.com/nyalldawson","followers_url":"https://api.github.com/users/nyalldawson/followers","following_url":"https://api.github.com/users/nyalldawson/following{/other_user}","gists_url":"https://api.github.com/users/nyalldawson/gists{/gist_id}","starred_url":"https://api.github.com/users/nyalldawson/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/nyalldawson/subscriptions","organizations_url":"https://api.github.com/users/nyalldawson/orgs","repos_url":"https://api.github.com/users/nyalldawson/repos","events_url":"https://api.github.com/users/nyalldawson/events{/privacy}","received_events_url":"https://api.github.com/users/nyalldawson/received_events","type":"User","site_admin":false},"milestone":null,"comments":19,"created_at":"2015-10-28T10:33:12Z","updated_at":"2015-11-09T20:28:29Z","closed_at":null,"body":"================================================================\r\nQGIS Enhancement 17: Required unit testing for \"critical\" classes\r\n================================================================\r\n\r\n**Date** 2015/10/20\r\n**Author** Nyall Dawson\r\n**Contact** nyall dot dawson at gmail dot com\r\n**Last Edited** 2015/10/20\r\n**Status**  Voting\r\n\r\n# Summary\r\n\r\nThis QEP describes a system of classifying certain classes within the QGIS codebase as critical\r\nclasses and proposes that no changes be allowed to these classes without accompanying unit\r\ntests.\r\n\r\nWhile QGIS contributor guidelines state that all changes to code within CORE be accompanied\r\nby unit tests, this requirement is universally ignored. However, since the introduction of CI\r\ntesting of every commit and pull request to master the value of writing tests to prevent\r\nregressions has tremendously increased. It is acknowledged that introduction of unit tests\r\ndoes not automatically fix bugs, but it does help drastically reduce the potential of regressions.\r\n\r\nThis QEP describes a middle-ground approach to requiring compulsory unit tests for certain areas\r\nof the codebase without placing too much additional burden on developers.\r\n\r\nIt is based on a well-defined minimal required test suite, and does not prevent\r\ndevelopers from writing additional tests or requiring tests for classes not\r\ncovered by this QEP.\r\n\r\n# Proposal\r\n\r\nAny change to a class which is classified as \"CRITICAL\" MUST be accompanied by \r\nunit testing to prevent regressions.\r\n\r\nPull requests which touch upon CRITICAL classes and which do not have unit\r\ntests must NOT be merged to master, and commits pushed to master which\r\nalso violate this will be reverted.\r\n\r\n## Definition of CRITICAL classes\r\n\r\nFor a class to be classified as a CRITICAL class it must satisfy two requirements:\r\n\r\n### The class must relate to code which has:\r\n\r\n- Potential for data loss for users\r\n- Risk of incorrect calculations resulting in invalid analysis (eg, incorrect area\r\nor distance calculations)\r\n\r\nThis excludes code which relates to purely cosmetic components, eg symbology,\r\nlabeling, conditional styles, etc. The requirement is intended to identify areas\r\nwhere regressions would result in significant harm to the reputation of QGIS\r\nor potential risk of incorrect analysis products generated by QGIS and the wider\r\nrisks this entails.\r\n\r\n### The class must have 100% unit test coverage (or as close as possible)\r\n\r\nThis requirement is intended to reduce the workload on developers. While it is\r\nrelatively straightforward to adopt existing unit tests to cover new code, it\r\ncan be quite burdensome or complex to start creation of unit tests from scratch.\r\nRequiring all changes to classes which satisfy 2.1.1 to have unit tests would likely\r\npush away potential contributors who do not have the time or skill required\r\nto write unit tests from scratch.\r\n\r\nAccordingly, a class can only be categorised as CRITICAL when it already has\r\n100% existing unit test coverage.\r\n\r\n# Proposed Technical Solution\r\n\r\nWhen a class is categorised as CRITICAL, comments will be added to the source\r\ncode to warn developers that all changes must be accompanied by unit tests.\r\nThese would take the form of the line:\r\n\r\n```\r\n// CRITICAL code - changes must be accompanied with unit tests in testqgsfield.cpp\r\n```\r\n\r\nplaced within the header, and between each function definition in the class cpp file.\r\n\r\nThe contributor guidelines would be updated to reflect this requirement.\r\n\r\nFor a class to be categorised as CRITICAL, a pull request will be opened which\r\nincludes the CRITICAL comment blocks for the class. The PR can then be used\r\nfor discussion regarding whether or not the class satisfies the requirements\r\nfor a critical class, specifically whether it satisfies 2.1.1.\r\n\r\n\r\n# Implementation Details\r\n\r\nInitially, only the QgsField, QgsFields and QgsFeature classes would satisfy\r\nthe requirements for CRITICAL classes. It is the intention that the following\r\nclasses be prioritised to bring them up to CRITICAL status ASAP:\r\n\r\n- all geometry classes\r\n- QgsExpression\r\n- QgsDistanceArea\r\n- QgsStatisticalSummary\r\n\r\n# Voting History\r\n\r\n+1\r\n\r\nAnita\r\n"},"comment":{"url":"https://api.github.com/repos/qgis/QGIS-Enhancement-Proposals/issues/comments/155183218","html_url":"https://github.com/qgis/QGIS-Enhancement-Proposals/issues/30#issuecomment-155183218","issue_url":"https://api.github.com/repos/qgis/QGIS-Enhancement-Proposals/issues/30","id":155183218,"user":{"login":"timlinux","id":178003,"avatar_url":"https://avatars.githubusercontent.com/u/178003?v=3","gravatar_id":"","url":"https://api.github.com/users/timlinux","html_url":"https://github.com/timlinux","followers_url":"https://api.github.com/users/timlinux/followers","following_url":"https://api.github.com/users/timlinux/following{/other_user}","gists_url":"https://api.github.com/users/timlinux/gists{/gist_id}","starred_url":"https://api.github.com/users/timlinux/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/timlinux/subscriptions","organizations_url":"https://api.github.com/users/timlinux/orgs","repos_url":"https://api.github.com/users/timlinux/repos","events_url":"https://api.github.com/users/timlinux/events{/privacy}","received_events_url":"https://api.github.com/users/timlinux/received_events","type":"User","site_admin":false},"created_at":"2015-11-09T20:28:29Z","updated_at":"2015-11-09T20:28:29Z","body":"@nyalldawson your amendment sounds good to me!"}}