QEP 17: Required unit testing for "critical" classes
Tim Sutton
Public
Seen by 243
We need to decide if we want to go ahead with QEP 17 which addresses mandatory testing of critical classes.
https://github.com/qgis/QGIS-Enhancement-Proposals/issues/30
Please keep the discussion related to this proposal in the comments for the QEP page itself.
Poll Created Wed 4 Nov 2015 8:49PM
We should require mandatory tests of core classes as per QEP #17 Closed Sat 7 Nov 2015 8:07PM
If you agree with the proposal as laid out in the QEP, please approve this.
Results
| Results | Option | % of points | Voters | |
|---|---|---|---|---|
|
|
Agree | 50.0% | 3 |
|
| Abstain | 50.0% | 3 |
|
|
| Disagree | 0.0% | 0 | ||
| Block | 0.0% | 0 | ||
| Undecided | 0% | 4 |
|
6 of 10 people have participated (60%)
Marco Hugentobler
Thu 5 Nov 2015 7:13AM
I don't mind. But if a PR is rejected because of missing unit tests, the dev should really get a meaningfull error message. Otherwise, it will prevent people from bringing their modifications into master branch.
Paolo Cavallini Thu 5 Nov 2015 2:48PM
Still uncertain. I think we should implement this gradualy, and adapt to what the outcome is.
Jürgen E. Fischer Fri 6 Nov 2015 5:18PM
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.
Tim Sutton Sat 7 Nov 2015 8:15AM
@nyall can I suggest you change the wording to make it clear that we will treat exceptions by their merits rather than making this a rigorously enforced rule.
Andreas Neumann · Thu 5 Nov 2015 7:51AM
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.