-1 for hard check, -1 for 100% coverage. Unittests are good but integration tests are better.
I totally agree with what Austin said. We should add tests where it makes sense. +1 soft check. I would not like finding myself banging my head [0] (just because of 100% coverage policy) against one line of code to cover which is not really realistic to happen, +1 complex to mock. +1 to own policy of plugins. [0] https://media.giphy.com/media/g8GfH3i5F0hby/giphy.gif -------- Regards, Ina Panova Software Engineer| Pulp| Red Hat Inc. "Do not go where the path may lead, go instead where there is no path and leave a trail." On Fri, Mar 23, 2018 at 6:42 PM, Austin Macdonald <aus...@redhat.com> wrote: > -1 For a blocking check, -1 for attempting 100% coverage. > > There is a *lot* of code in Pulp 3 that simply involves inheriting from > parents classes and defining attributes. If we add a ViewSet for instance, > there is nothing to test other than "asserting that we did what we did". I > have written lots of tests like that. IMO, they add no value and are time > consuming to write. Also, they are time consuming to maintain because every > change must also change the unit tests. This type of test almost never > catches a real problem. > > A soft check would be a useful reminder to the contributor and the > reviewer to add tests where they make sense. + 1 soft check > > Plugins: Each plugin team should determine their own unit test policy. > > > On Fri, Mar 23, 2018 at 1:26 PM, David Davis <davidda...@redhat.com> > wrote: > >> We haven't had a unit test policy in Pulp 3, and the community and core >> committers would all like one. The general desire we've heard so far is to >> change course and encourage developers to add unit tests to their changes >> to Pulp 3. >> >> The policy we're suggesting is to add a coveralls[0] check for Pull >> Requests against the pulpcore 3.0-dev branch that shows the overall >> coverage percentage, e.g. 12.89%. This check would pass if and only if >> coverage increases or remains the same with the PR. We think this will >> eventually get us on the path to 100% unit test coverage. >> >> We propose the coveralls check be a soft check that allow for merging if >> it fails. We would document the policy and try to adhere to it even though >> it wouldn't formally block merging. At a future point when pulp3 (maybe the >> GA?) we could make this a hard check. >> >> Benefits: >> - It's easy, simple, and automatic >> - It's pretty objective and there's little room to argue with a number. >> - Helps us raise our unit test coverage gradually over time >> >> Downsides: >> - Could discourage community contributions >> - It can be a bit strict and unforgiving at times (especially if there's >> a hard check) >> - It only provides a guarantee around quantity of unit testing and not >> quality >> >> >> *Q: What about the existing functionality? Do we need to write unit tests >> for it?* >> >> We're not sure about this. We'd like community feedback. Is anyone >> interested in writing backfill unit tests? If so, then maybe we should. >> >> *Q: Will this also affect Pulp 2?* >> >> We're not planning on it but if we like this enough, we can look at >> adding it to Pulp 2. >> >> *Q: Will this affect plugins?* >> >> We want to start out with just pulpcore now and see how we like it. Then >> we'll look at adding it to pulp_file. In the future, we can also look at >> ways to make it easy for plugins to set this up. >> >> *Q: Do I no longer need to write pulp-smash test plan issues in Github >> for Pulp 3 features?* >> >> No, you should still do that. >> >> [0] https://coveralls.io/ >> >> _______________________________________________ >> Pulp-dev mailing list >> Pulp-dev@redhat.com >> https://www.redhat.com/mailman/listinfo/pulp-dev >> >> > > _______________________________________________ > Pulp-dev mailing list > Pulp-dev@redhat.com > https://www.redhat.com/mailman/listinfo/pulp-dev > >
_______________________________________________ Pulp-dev mailing list Pulp-dev@redhat.com https://www.redhat.com/mailman/listinfo/pulp-dev
