Re: [openstack-dev] [neutron] Generalized issues in the unit testing of ML2 mechanism drivers

[Isaku Yamahata]

On Tue, Feb 13, 2018 at 05:48:32PM -0500,
Assaf Muller  wrote:

> On Wed, Dec 13, 2017 at 7:30 AM, Michel Peterson  wrote:
> > Through my work in networking-odl I've found what I believe is an issue
> > present in a majority of ML2 drivers. An issue I think needs awareness so
> > each project can decide a course of action.
> >
> > The issue stems from the adopted practice of importing
> > `neutron.tests.unit.plugins.ml2.test_plugin` and creating classes with noop
> > operation to "inherit" tests for free [1]. The idea behind is nice, you
> > inherit >600 tests that cover several scenarios.
> >
> > There are several issues of adopting this pattern, two of which are
> > paramount:
> >
> > 1. If the mechanism driver is not loaded correctly [2], the tests then don't
> > test the mechanism driver but still succeed and therefore there is no
> > indication that there is something wrong with the code. In the case of
> > networking-odl it wasn't discovered until last week, which means that for >1
> > year it this was adding PASSed tests uselessly.
> >
> > 2. It gives a false sense of reassurance. If the code of those tests is
> > analyzed it's possible to see that the code itself is mostly centered around
> > testing the REST endpoint of neutron than actually testing that the
> > mechanism succeeds on the operation it was supposed to test. As a result of
> > this, there is marginally added value on having those tests. To be clear,
> > the hooks for the respective operations are called on the mechanism driver,
> > but the result of the operation is not asserted.
> >
> > I would love to hear more voices around this, so feel free to comment.
> >
> > Regarding networking-odl the solution I propose is the following:
> >   **First**, discard completely the change mentioned in the footnote #2.
> >   **Second**, create a patch that completely removes the tests that follow
> > this pattern.
> 
> An interesting exercise would be to add 'raise ValueError' type
> exceptions in various ODL ML2 mech driver flows and seeing which tests
> fail. Basically, if a test passes without the ODL mech driver loaded,
> or with a faulty ODL mech driver, then you don't need to run the test
> for networking-odl changes. I'd be hesitant to remove all tests
> though, it's a good investment of time to figure out which tests are
> valuable to you.

Mike and Michel should raise it at the PTG for discussion.
I know Mike will attend.

thanks,
-- 
Isaku Yamahata 

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev

Re: [openstack-dev] [neutron] Generalized issues in the unit testing of ML2 mechanism drivers

[Assaf Muller]

On Wed, Dec 13, 2017 at 7:30 AM, Michel Peterson  wrote:
> Through my work in networking-odl I've found what I believe is an issue
> present in a majority of ML2 drivers. An issue I think needs awareness so
> each project can decide a course of action.
>
> The issue stems from the adopted practice of importing
> `neutron.tests.unit.plugins.ml2.test_plugin` and creating classes with noop
> operation to "inherit" tests for free [1]. The idea behind is nice, you
> inherit >600 tests that cover several scenarios.
>
> There are several issues of adopting this pattern, two of which are
> paramount:
>
> 1. If the mechanism driver is not loaded correctly [2], the tests then don't
> test the mechanism driver but still succeed and therefore there is no
> indication that there is something wrong with the code. In the case of
> networking-odl it wasn't discovered until last week, which means that for >1
> year it this was adding PASSed tests uselessly.
>
> 2. It gives a false sense of reassurance. If the code of those tests is
> analyzed it's possible to see that the code itself is mostly centered around
> testing the REST endpoint of neutron than actually testing that the
> mechanism succeeds on the operation it was supposed to test. As a result of
> this, there is marginally added value on having those tests. To be clear,
> the hooks for the respective operations are called on the mechanism driver,
> but the result of the operation is not asserted.
>
> I would love to hear more voices around this, so feel free to comment.
>
> Regarding networking-odl the solution I propose is the following:
>   **First**, discard completely the change mentioned in the footnote #2.
>   **Second**, create a patch that completely removes the tests that follow
> this pattern.

An interesting exercise would be to add 'raise ValueError' type
exceptions in various ODL ML2 mech driver flows and seeing which tests
fail. Basically, if a test passes without the ODL mech driver loaded,
or with a faulty ODL mech driver, then you don't need to run the test
for networking-odl changes. I'd be hesitant to remove all tests
though, it's a good investment of time to figure out which tests are
valuable to you.

>   **Third**, incorporate the neutron tempest plugin into the CI and rely on
> that for assuring coverage of the different scenarios.
>
> Also to mention that when discovered this issue in networking-odl we took a
> decision not to merge more patches until the PS of footnote #2 was
> addressed. I think we can now decide to overrule that decision and proceed
> as usual.
>
>
>
> [1]: http://codesearch.openstack.org/?q=class%20.*\(.*TestMl2
> [2]: something that was happening in networking-odl and addressed by
> https://review.openstack.org/#/c/523934
>
> __
> OpenStack Development Mailing List (not for usage questions)
> Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>

__
OpenStack Development Mailing List (not for usage questions)
Unsubscribe: openstack-dev-requ...@lists.openstack.org?subject:unsubscribe
http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev