Re: [Pulp-dev] Pulp 3 Unit Test Plan Proposal

2018-04-06 Thread Robin Chan
Thank you David & Brian on driving this plan forward.

I am happy we will release a Pulp 3.0 Core Beta with a known % coverage for
unit tests, a policy in place and mechanisms in place to ensure that number
only goes up.

In the future, I would like for us to flush out more the definition of
valuable unit tests vs. "not valuable," since I heard this a lot
generically but not spelled out in any way. I think this would be a good
way to provide transparency and ensure that when the % goes up in a
valuable way, but I don't believe that to be highest on our priority list.

Thank you!
Robin


On Fri, Apr 6, 2018 at 2:57 PM, Brian Bouterse  wrote:

> There was a lot of +1 support on the PR. The basic Pulp3 unit test policy
> can be seen here: https://docs.pulpproject.org/
> en/3.0/nightly/contributing/unit_tests.html
>
> On Thu, Apr 5, 2018 at 1:06 PM, Brian Bouterse 
> wrote:
>
>> This is a wrap-up update with the last next steps (for now) on the Pulp3
>> unit test discussion.
>>
>> 1. Here is a docs PR adopting simple but specific policy language
>> recommending unit tests for Pulp3: https://github.com/pulp/pulp/pull/3411
>> 2. We need to move our existing unit tests into their "forever home" so
>> @daviddavis and I groomed this issue and put it on the sprint:
>> https://pulp.plan.io/issues/3553
>>
>> For any areas of Pulp3 untested code that you want to add unit tests for
>> please file a Task in Redmine for that. A few of those have been filed AIUI.
>>
>> If there are any issues with these changes please bring them up. Any
>> aspect of them can be rethought. David and I have tried to facilitate what
>> we've heard from the group.
>>
>>
>>
>> On Mon, Mar 26, 2018 at 5:13 PM, Brian Bouterse 
>> wrote:
>>
>>> I want to summarize what I've heard to facilitate some next steps and
>>> further discussion.
>>>
>>> There seems to be broad support and no -1 votes to the idea of a soft
>>> check that tracks unit test coverage, so we wanted to get that out of the
>>> way. Daviddavis enabled unit test coverage reporting for all Pulp3 PRs (
>>> https://github.com/pulp/pulp/pull/3397) and it will report on all PRs
>>> now. Currently, it shows 54.98% for pulpcore. That number is surprisingly
>>> high but not awesome. When looking at the report, it is mainly all import
>>> statements and function definitions since we have few/zero unit tests but
>>> also not much code.
>>>
>>> Based on feedback it sounds like leaving it at a soft check and highly
>>> encouraging unit tests with each PR is something we could all agree on. I
>>> want us to get to specific language that we can add into the Pulp3 docs as
>>> a new section called "Unit Tests" here: https://docs.pulpproject.org/e
>>> n/3.0/nightly/contributing/index.html Here is a starting point, please
>>> send suggestions:  "All new code is highly encouraged to have basic unit
>>> tests that demonstrate its functionality. A Pull Request that has failing
>>> unit tests cannot be merged."
>>>
>>>
>>> Also from the convo on-list and on-irc, here are some questions I would
>>> like help answering:
>>>
>>> * What areas in the existing codebase would really benefit from unit
>>> testing? I think we need a classpath list of modules and classes. I made an
>>> etherpad here: https://etherpad.net/p/Pulp_Unit_Test_Candidates
>>>
>>> * What are the existing unit tests and where do they live?
>>>
>>> * What docs need to be added to make contributing unit tests reasonable?
>>>
>>>
>>> Thanks for all the discussion!
>>> -Brian
>>>
>>>
>>> On Mon, Mar 26, 2018 at 4:01 PM, Jeremy Audet  wrote:
>>>
 > I'm also generally -1 against trying to pick a number (100%, 80%,
 60%) up-front.  We should unit test what makes sense to unit test, push
 that number as high as reasonable, and otherwise focus on pulp-smash, which
 I think has historically been more useful.

 QE is flattered by the focus on Pulp Smash. We're happy that the smoke
 tests are being executed as a pull request check.

 However, it's important to remember that unit tests are far faster
 than integration tests, typically by several orders of magnitude. In
 addition, Pulp Smash's smoke tests are intentionally limited. They're
 designed to execute quickly and to detect catastrophic regressions.
 They're not intended to be comprehensive. In fact, some of the
 already-written test cases may be stripped of their "smoke test"
 status for the sake of speed. Psychology is important here: it's bad
 if a developer locally fires off smoke tests, gets bored, and opens up
 a new web browser tab.

 Please keep this limitation in mind when deciding on policies
 regarding smoke tests.

 ___
 Pulp-dev mailing list
 Pulp-dev@redhat.com
 https://www.redhat.com/mailman/listinfo/pulp-dev

>>>
>>>
>>
>
> 

Re: [Pulp-dev] Pulp 3 Unit Test Plan Proposal

2018-04-06 Thread Brian Bouterse
There was a lot of +1 support on the PR. The basic Pulp3 unit test policy
can be seen here:
https://docs.pulpproject.org/en/3.0/nightly/contributing/unit_tests.html

On Thu, Apr 5, 2018 at 1:06 PM, Brian Bouterse  wrote:

> This is a wrap-up update with the last next steps (for now) on the Pulp3
> unit test discussion.
>
> 1. Here is a docs PR adopting simple but specific policy language
> recommending unit tests for Pulp3: https://github.com/pulp/pulp/pull/3411
> 2. We need to move our existing unit tests into their "forever home" so
> @daviddavis and I groomed this issue and put it on the sprint:
> https://pulp.plan.io/issues/3553
>
> For any areas of Pulp3 untested code that you want to add unit tests for
> please file a Task in Redmine for that. A few of those have been filed AIUI.
>
> If there are any issues with these changes please bring them up. Any
> aspect of them can be rethought. David and I have tried to facilitate what
> we've heard from the group.
>
>
>
> On Mon, Mar 26, 2018 at 5:13 PM, Brian Bouterse 
> wrote:
>
>> I want to summarize what I've heard to facilitate some next steps and
>> further discussion.
>>
>> There seems to be broad support and no -1 votes to the idea of a soft
>> check that tracks unit test coverage, so we wanted to get that out of the
>> way. Daviddavis enabled unit test coverage reporting for all Pulp3 PRs (
>> https://github.com/pulp/pulp/pull/3397) and it will report on all PRs
>> now. Currently, it shows 54.98% for pulpcore. That number is surprisingly
>> high but not awesome. When looking at the report, it is mainly all import
>> statements and function definitions since we have few/zero unit tests but
>> also not much code.
>>
>> Based on feedback it sounds like leaving it at a soft check and highly
>> encouraging unit tests with each PR is something we could all agree on. I
>> want us to get to specific language that we can add into the Pulp3 docs as
>> a new section called "Unit Tests" here: https://docs.pulpproject.org/e
>> n/3.0/nightly/contributing/index.html Here is a starting point, please
>> send suggestions:  "All new code is highly encouraged to have basic unit
>> tests that demonstrate its functionality. A Pull Request that has failing
>> unit tests cannot be merged."
>>
>>
>> Also from the convo on-list and on-irc, here are some questions I would
>> like help answering:
>>
>> * What areas in the existing codebase would really benefit from unit
>> testing? I think we need a classpath list of modules and classes. I made an
>> etherpad here: https://etherpad.net/p/Pulp_Unit_Test_Candidates
>>
>> * What are the existing unit tests and where do they live?
>>
>> * What docs need to be added to make contributing unit tests reasonable?
>>
>>
>> Thanks for all the discussion!
>> -Brian
>>
>>
>> On Mon, Mar 26, 2018 at 4:01 PM, Jeremy Audet  wrote:
>>
>>> > I'm also generally -1 against trying to pick a number (100%, 80%, 60%)
>>> up-front.  We should unit test what makes sense to unit test, push that
>>> number as high as reasonable, and otherwise focus on pulp-smash, which I
>>> think has historically been more useful.
>>>
>>> QE is flattered by the focus on Pulp Smash. We're happy that the smoke
>>> tests are being executed as a pull request check.
>>>
>>> However, it's important to remember that unit tests are far faster
>>> than integration tests, typically by several orders of magnitude. In
>>> addition, Pulp Smash's smoke tests are intentionally limited. They're
>>> designed to execute quickly and to detect catastrophic regressions.
>>> They're not intended to be comprehensive. In fact, some of the
>>> already-written test cases may be stripped of their "smoke test"
>>> status for the sake of speed. Psychology is important here: it's bad
>>> if a developer locally fires off smoke tests, gets bored, and opens up
>>> a new web browser tab.
>>>
>>> Please keep this limitation in mind when deciding on policies
>>> regarding smoke tests.
>>>
>>> ___
>>> 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


Re: [Pulp-dev] Pulp 3 Unit Test Plan Proposal

2018-04-05 Thread Brian Bouterse
This is a wrap-up update with the last next steps (for now) on the Pulp3
unit test discussion.

1. Here is a docs PR adopting simple but specific policy language
recommending unit tests for Pulp3: https://github.com/pulp/pulp/pull/3411
2. We need to move our existing unit tests into their "forever home" so
@daviddavis and I groomed this issue and put it on the sprint:
https://pulp.plan.io/issues/3553

For any areas of Pulp3 untested code that you want to add unit tests for
please file a Task in Redmine for that. A few of those have been filed AIUI.

If there are any issues with these changes please bring them up. Any aspect
of them can be rethought. David and I have tried to facilitate what we've
heard from the group.



On Mon, Mar 26, 2018 at 5:13 PM, Brian Bouterse  wrote:

> I want to summarize what I've heard to facilitate some next steps and
> further discussion.
>
> There seems to be broad support and no -1 votes to the idea of a soft
> check that tracks unit test coverage, so we wanted to get that out of the
> way. Daviddavis enabled unit test coverage reporting for all Pulp3 PRs (
> https://github.com/pulp/pulp/pull/3397) and it will report on all PRs
> now. Currently, it shows 54.98% for pulpcore. That number is surprisingly
> high but not awesome. When looking at the report, it is mainly all import
> statements and function definitions since we have few/zero unit tests but
> also not much code.
>
> Based on feedback it sounds like leaving it at a soft check and highly
> encouraging unit tests with each PR is something we could all agree on. I
> want us to get to specific language that we can add into the Pulp3 docs as
> a new section called "Unit Tests" here: https://docs.pulpproject.org/
> en/3.0/nightly/contributing/index.html Here is a starting point, please
> send suggestions:  "All new code is highly encouraged to have basic unit
> tests that demonstrate its functionality. A Pull Request that has failing
> unit tests cannot be merged."
>
>
> Also from the convo on-list and on-irc, here are some questions I would
> like help answering:
>
> * What areas in the existing codebase would really benefit from unit
> testing? I think we need a classpath list of modules and classes. I made an
> etherpad here: https://etherpad.net/p/Pulp_Unit_Test_Candidates
>
> * What are the existing unit tests and where do they live?
>
> * What docs need to be added to make contributing unit tests reasonable?
>
>
> Thanks for all the discussion!
> -Brian
>
>
> On Mon, Mar 26, 2018 at 4:01 PM, Jeremy Audet  wrote:
>
>> > I'm also generally -1 against trying to pick a number (100%, 80%, 60%)
>> up-front.  We should unit test what makes sense to unit test, push that
>> number as high as reasonable, and otherwise focus on pulp-smash, which I
>> think has historically been more useful.
>>
>> QE is flattered by the focus on Pulp Smash. We're happy that the smoke
>> tests are being executed as a pull request check.
>>
>> However, it's important to remember that unit tests are far faster
>> than integration tests, typically by several orders of magnitude. In
>> addition, Pulp Smash's smoke tests are intentionally limited. They're
>> designed to execute quickly and to detect catastrophic regressions.
>> They're not intended to be comprehensive. In fact, some of the
>> already-written test cases may be stripped of their "smoke test"
>> status for the sake of speed. Psychology is important here: it's bad
>> if a developer locally fires off smoke tests, gets bored, and opens up
>> a new web browser tab.
>>
>> Please keep this limitation in mind when deciding on policies
>> regarding smoke tests.
>>
>> ___
>> 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


Re: [Pulp-dev] Pulp 3 Unit Test Plan Proposal

2018-03-26 Thread Brian Bouterse
I want to summarize what I've heard to facilitate some next steps and
further discussion.

There seems to be broad support and no -1 votes to the idea of a soft check
that tracks unit test coverage, so we wanted to get that out of the way.
Daviddavis enabled unit test coverage reporting for all Pulp3 PRs (
https://github.com/pulp/pulp/pull/3397) and it will report on all PRs now.
Currently, it shows 54.98% for pulpcore. That number is surprisingly high
but not awesome. When looking at the report, it is mainly all import
statements and function definitions since we have few/zero unit tests but
also not much code.

Based on feedback it sounds like leaving it at a soft check and highly
encouraging unit tests with each PR is something we could all agree on. I
want us to get to specific language that we can add into the Pulp3 docs as
a new section called "Unit Tests" here:
https://docs.pulpproject.org/en/3.0/nightly/contributing/index.html Here is
a starting point, please send suggestions:  "All new code is highly
encouraged to have basic unit tests that demonstrate its functionality. A
Pull Request that has failing unit tests cannot be merged."


Also from the convo on-list and on-irc, here are some questions I would
like help answering:

* What areas in the existing codebase would really benefit from unit
testing? I think we need a classpath list of modules and classes. I made an
etherpad here: https://etherpad.net/p/Pulp_Unit_Test_Candidates

* What are the existing unit tests and where do they live?

* What docs need to be added to make contributing unit tests reasonable?


Thanks for all the discussion!
-Brian


On Mon, Mar 26, 2018 at 4:01 PM, Jeremy Audet  wrote:

> > I'm also generally -1 against trying to pick a number (100%, 80%, 60%)
> up-front.  We should unit test what makes sense to unit test, push that
> number as high as reasonable, and otherwise focus on pulp-smash, which I
> think has historically been more useful.
>
> QE is flattered by the focus on Pulp Smash. We're happy that the smoke
> tests are being executed as a pull request check.
>
> However, it's important to remember that unit tests are far faster
> than integration tests, typically by several orders of magnitude. In
> addition, Pulp Smash's smoke tests are intentionally limited. They're
> designed to execute quickly and to detect catastrophic regressions.
> They're not intended to be comprehensive. In fact, some of the
> already-written test cases may be stripped of their "smoke test"
> status for the sake of speed. Psychology is important here: it's bad
> if a developer locally fires off smoke tests, gets bored, and opens up
> a new web browser tab.
>
> Please keep this limitation in mind when deciding on policies
> regarding smoke tests.
>
> ___
> 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


Re: [Pulp-dev] Pulp 3 Unit Test Plan Proposal

2018-03-26 Thread Jeremy Audet
> I'm also generally -1 against trying to pick a number (100%, 80%, 60%) 
> up-front.  We should unit test what makes sense to unit test, push that 
> number as high as reasonable, and otherwise focus on pulp-smash, which I 
> think has historically been more useful.

QE is flattered by the focus on Pulp Smash. We're happy that the smoke
tests are being executed as a pull request check.

However, it's important to remember that unit tests are far faster
than integration tests, typically by several orders of magnitude. In
addition, Pulp Smash's smoke tests are intentionally limited. They're
designed to execute quickly and to detect catastrophic regressions.
They're not intended to be comprehensive. In fact, some of the
already-written test cases may be stripped of their "smoke test"
status for the sake of speed. Psychology is important here: it's bad
if a developer locally fires off smoke tests, gets bored, and opens up
a new web browser tab.

Please keep this limitation in mind when deciding on policies
regarding smoke tests.

___
Pulp-dev mailing list
Pulp-dev@redhat.com
https://www.redhat.com/mailman/listinfo/pulp-dev


Re: [Pulp-dev] Pulp 3 Unit Test Plan Proposal

2018-03-26 Thread Daniel Alley
We would still block on failing tests, yes.

I'm also -1 blocking on coverage, and -1 against attempting 100%.

I'm also generally -1 against trying to pick a number (100%, 80%, 60%)
up-front.  We should unit test what makes sense to unit test, push that
number as high as reasonable, and otherwise focus on pulp-smash, which I
think has historically been more useful.

On Mon, Mar 26, 2018 at 12:31 PM, Bryan Kearney  wrote:

> I assume you would still gate (hard fail) if unit tests fail?
>
> -- bk
>
> On 03/26/2018 05:55 AM, Ina Panova wrote:
>
>> -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 > > 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 > > 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
>> 

Re: [Pulp-dev] Pulp 3 Unit Test Plan Proposal

2018-03-26 Thread Bryan Kearney

I assume you would still gate (hard fail) if unit tests fail?

-- bk

On 03/26/2018 05:55 AM, Ina Panova wrote:

-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 > 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 > 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



___
Pulp-dev mailing list
Pulp-dev@redhat.com
https://www.redhat.com/mailman/listinfo/pulp-dev


Re: [Pulp-dev] Pulp 3 Unit Test Plan Proposal

2018-03-26 Thread Ina Panova
-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  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 
> 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


Re: [Pulp-dev] Pulp 3 Unit Test Plan Proposal

2018-03-23 Thread Austin Macdonald
-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  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