On 7/23/14, 7:51 AM, "Steve Baker" <sba...@redhat.com> wrote:
>On 18/07/14 08:35, Matt Riedemann wrote: >> >> >> On 7/17/2014 5:48 PM, Steve Baker wrote: >>> On 18/07/14 00:44, Joe Gordon wrote: >>>> >>>> >>>> >>>> On Wed, Jul 16, 2014 at 11:28 PM, Steve Baker <sba...@redhat.com >>>> <mailto:sba...@redhat.com>> wrote: >>>> >>>> On 12/07/14 09:25, Joe Gordon wrote: >>>>> >>>>> >>>>> >>>>> On Fri, Jul 11, 2014 at 4:42 AM, Jeremy Stanley >>>>> <fu...@yuggoth.org <mailto:fu...@yuggoth.org>> wrote: >>>>> >>>>> On 2014-07-11 11:21:19 +0200 (+0200), Matthias Runge wrote: >>>>> > this broke horizon stable and master; heat stable is >>>>> affected as >>>>> > well. >>>>> [...] >>>>> >>>>> I guess this is a plea for applying something like the >>>>> oslotest >>>>> framework to client libraries so they get backward-compat >>>>> jobs run >>>>> against unit tests of all dependant/consuming software... >>>>> branchless >>>>> tempest already alleviates some of this, but not the case of >>>>> changes >>>>> in a library which will break unit/functional tests of >>>>>another >>>>> project. >>>>> >>>>> >>>>> We actually do have some tests for backwards compatibility, and >>>>> they all passed. Presumably because both heat and horizon have >>>>> poor integration test. >>>>> >>>>> We ran >>>>> >>>>> * check-tempest-dsvm-full-havana >>>>> >>>>> >>>>><http://logs.openstack.org/66/94166/3/check/check-tempest-dsvm-full-ha >>>>>vana/8e09faa> >>>>> SUCCESS in 40m 47s (non-voting) >>>>> * check-tempest-dsvm-neutron-havana >>>>> >>>>> >>>>><http://logs.openstack.org/66/94166/3/check/check-tempest-dsvm-neutron >>>>>-havana/b4ad019> >>>>> SUCCESS in 36m 17s (non-voting) >>>>> * check-tempest-dsvm-full-icehouse >>>>> >>>>> >>>>><http://logs.openstack.org/66/94166/3/check/check-tempest-dsvm-full-ic >>>>>ehouse/c0c62e5> >>>>> SUCCESS in 53m 05s >>>>> * check-tempest-dsvm-neutron-icehouse >>>>> >>>>> >>>>><http://logs.openstack.org/66/94166/3/check/check-tempest-dsvm-neutron >>>>>-icehouse/a54aedb> >>>>> SUCCESS in 57m 28s >>>>> >>>>> >>>>> on the offending patches >>>>>(https://review.openstack.org/#/c/94166/) >>>>> >>>>> Infra patch that added these tests: >>>>> https://review.openstack.org/#/c/80698/ >>>>> >>>>> >>>> Heat-proper would have continued working fine with novaclient >>>> 2.18.0. The regression was with raising novaclient exceptions, >>>> which is only required in our unit tests. I saw this break coming >>>> and switched to raising via from_response >>>> https://review.openstack.org/#/c/97977/22/heat/tests/v1_1/fakes.py >>>> >>>> Unit tests tend to deal with more internals of client libraries >>>> just for mocking purposes, and there have been multiple breaks in >>>> unit tests for heat and horizon when client libraries make >>>> internal changes. >>>> >>>> This could be avoided if the client gate jobs run the unit tests >>>> for the projects which consume them. >>>> >>>> That may work but isn't this exactly what integration testing is for? >>> If you mean tempest then no, this is different. >>> >>> Client projects have done a good job of keeping their public library >>> APIs stable. An exception type is public API, but the constructor for >>> raising that type arguably is more of a gray area since only the client >>> library should be raising its own exceptions. >>> >>> However heat and horizon unit tests need to raise client exceptions to >>> test their own error condition handling, so exception constructors >>>could >>> be considered public API, but only for unit test mocking in other >>> projects. >>> >>> This problem couldn't have been caught in an integration test because >>> nothing outside the unit tests directly raises a client exception. >>> >>> There have been other breakages where internal client library changes >>> have broken the mocking in our unit tests (I recall a neutronclient >>> internal refactor). >>> >>> In many cases the cause may be inappropriate mocking in the unit tests, >>> but that is cold comfort when the gates break when a client library is >>> released. >>> >>> Maybe we can just start with adding heat and horizon to the check jobs >>> of the clients they consume, but the following should also be >>> considered: >>> grep "python-.*client" */requirements.txt >>> >>> This could give client libraries more confidence that internal changes >>> don't break anything, and allows them to fix mocking in other projects >>> before their changes land. >>> >>> >>> >>> _______________________________________________ >>> OpenStack-dev mailing list >>> OpenStack-dev@lists.openstack.org >>> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev >>> >> >> I don't think we should have to change the gate jobs just so that >> other projects can test against the internals of their dependent >> clients, that sounds like a flawed unit test design to me. >> >> Looking at >> https://review.openstack.org/#/c/97977/22/heat/tests/v1_1/fakes.py for >> example, why is a fake_exception needed to mock out novaclient's >> NotFound exception? A better way to do this is that whatever is >> expecting to raise the NotFound should use mock with a side_effect to >> raise novaclient.exceptions.NotFound, then mock handles the spec being >> set on the mock and you don't have to worry about the internal >> construction of the exception class in your unit tests. >> >fakes.py is ancient and doesn't reflect how we write tests currently. >Our mox tests use AssertRaises (less ancient) and our mock tests use >side_effect (current). However all three approaches are in the same >situation when it is not possible to construct a NotFound object in a >way which works with both novaclient 2.17.0 and 2.18.0. > >And we've just been hit by this again - our progress on getting heat >juno-2 released stopped when keystoneclient 0.10.0 was released, which >required an urgent fix [1] > >This is the kind of change which would be discovered early if heat and >horizon unit tests ran against python-*client changes. I don't even >think these jobs should be voting, but they'll give us enough early >warning that our mocking technique needs revisiting. They will also give >client developers an idea of the impact of their changes, and allow them >to consider whether they are actually changing public API. > >[1] https://review.openstack.org/#/c/108875/1 > >_______________________________________________ >OpenStack-dev mailing list >OpenStack-dev@lists.openstack.org >http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev Horizon was also bitten by this latest release and required an urgent fix [1]. I fully agree there needs to be at least non-voting tests using master on the various client repos and other repos like django_openstack_auth to provide a earlier notification of potential issues. The latest issue in Horizon was due to the utilization of client internals which was poorly thought out, but a notification before the all Horizon tests ground to a halt would have been a vast improvement. [1] https://review.openstack.org/108865 _______________________________________________ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev