Re: [openstack-dev] [neutron] Generalized issues in the unit testing of ML2 mechanism drivers
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
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
[openstack-dev] [neutron] Generalized issues in the unit testing of ML2 mechanism drivers
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. **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
