Open Stack

On Sun, Feb 10, 2013 at 3:21 PM, Monty Taylor <mordred at inaugust.com> wrote:

>
>
> On 02/10/2013 11:37 AM, John Griffith wrote:
> >
> >
> > On Sun, Feb 10, 2013 at 3:03 AM, Huang Zhiteng <winston.d at gmail.com
> > <mailto:winston.d at gmail.com>> wrote:
> >
> >     John, Monty, Mark,
> >
> >     I think one example is unit tests involving test case against entry
> >     points which requires installation.  I guess that's John's most
> >     annoying thing.
> >
> >     For this special case, I think switching from run_tests.sh to tox (or
> >     even ditching run_tests.sh) is the right solution.  I'm working on a
> >     patch for Cinder to make this change
> >     (https://review.openstack.org/#/c/21597/).   It's not ready yet as I
> >     met some difficulty with coverage tests, but as a starter, you can
> try
> >     functionality other than coverage tests.
> >
> >     On Sun, Feb 10, 2013 at 5:23 AM, Monty Taylor <mordred at inaugust.com
> >     <mailto:mordred at inaugust.com>> wrote:
> >     >
> >     >
> >     > On 02/09/2013 01:59 PM, Mark McLoughlin wrote:
> >     >> Hi John,
> >     >>
> >     >> On Sat, 2013-02-09 at 12:15 -0700, John Griffith wrote:
> >     >>> We seem to be going down a path of requiring more and more
> >     infrastructure
> >     >>> and reliance from modules inside of the respective projects code
> >     base, and
> >     >>> lately (the most annoying thing to me) is actually requiring the
> >     >>> installation of the project and it's services in order for our
> >     unit tests
> >     >>> to run.
> >     >>
> >     >> Got an example?
> >     >
> >     > Yes. I'd also like an example.
> >     >
> >     > FWIW - I agree with the sentiment that our unittests should be
> >     simple to
> >     > run and that cross-project testing should be the purview of
> >     integration
> >     > tests.
> >     >
> >     > Monty
> >     >
> >     > _______________________________________________
> >     > OpenStack-dev mailing list
> >     > OpenStack-dev at lists.openstack.org
> >     <mailto:OpenStack-dev at lists.openstack.org>
> >     > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
> >
> >
> >
> >     --
> >     Regards
> >     Huang Zhiteng
> >
> >     _______________________________________________
> >     OpenStack-dev mailing list
> >     OpenStack-dev at lists.openstack.org
> >     <mailto:OpenStack-dev at lists.openstack.org>
> >     http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
> >
> >
> > Sorry for dropping the email then going MIA.
> >
> > There are multiple things I wanted to get some thought around, I'll
> > start with what I believe are the easier ones first:
> >
> > 1. I don't believe that we should require the installation of Cinder to
> > run the Cinder unit tests
> >         This to me clearly crosses the line in to functional or
> > integration territory, and moving it in to
> >         a venv (tox, or otherwise) doesn't address my issue with it.
> > Sorry Winston but right now 21597 does
> >         exactly what I don't want.
> >
> >         Today for Cinder I have the test-requires packages and other
> > required items all installed on my laptop and other machines.
> >         I can clone the Cinder repo and run all of the tests using
> > 'run_tests.sh -N' in less than a minute.  Lately unit tests are coming
> >         online that depend on the egg info being installed as Winston
> > touched on.
>
> I've got a patch coming that will help with that.
>

Cool, I started looking at the version change and how to get around this
already but I'll let you finish it :)


>
> > 2. I haven't figured out what Tox versus Nose buys me in Cinder.
> >         I understand in Nova and others there may be advantages due to
> > parallelism and some other enhancements, but in my
> >         case my test cycle time has actually increased by having to
> > install the tox venv when I do a fresh clone, and in the case
> >         of Nova, I *think* my tests are running a bit faster but really
> > I don't feel that I actually know what's being run and the results.
>
> tox vs. nose isn't a thing. There are two sets of things:
>
> a) nose vs. testr - this is the thing with parallelism, and also with
> using a test runner that doesn't inject evil into things. When we
> propose a patch to cinder to migrate you to testr, we will propose a
> patch to run_tests.sh that will run testr instead of nose, and your
> above referenced workflow will work as usual and will get faster and
> more consistent.
>
> b) tox vs. run_tests - this is about how virtualenvs are created and
> managed. The use of tox has been enabled in cinder for as long as it has
> existed and nothing has changed with that recently. If you run
> run_tests, nothing about tox will interface with you life.
>
> However - honestly, if what you want to do is install the requirements
> locally, not use virtualenvs and run things quickly, once we get testr
> landed, you should stop using run_tests.sh and just run testr directly.
> But it's not required.
>

Ok, that makes sense, thanks for educating me on that. I personally could
care less what the command I use is, it's the basic work flow that I'm a
big fan of.  I have been trying to get this working correctly in Nova and
think I have it sorted using "testr run", and that's fine WRT to this
particular item.


>
> > These first two items are the main things I'm looking for feed-back from
> > folks on, the rest of this email is more around test philosophy, and I
> > don't think I'm interested in trying to come to an agreement or change
> > on things here.  I'd just like to raise some awareness, if anybody is
> > interested in pursuing some of this, maybe we can all get together and
> > chat at the summit.  If people think it's ridiculous that's fine too.
>
> The "must install cinder to run unit tests" is a bug. Patch is on its
> way... (and sorry, there's about 100 use cases up in there and we missed
> one when we did the recent version code)
>

So TBF here, I wasn't even going to mention the version patch.  I happened
to miss this in my review because coincidentally I was working in devstack
when I saw your patch and just pulled it and ran it there so everything was
fine.  On the other hand, there have been patches submitted however that
intentionally require cinder to be installed, I've let one go through and
added a skip if cinder not installed, and other's I've pushed back and said
'thanks but no thanks".  I feel the same way about 3'rd party storage
device vendors requiring pip install of their library/packages for unit
tests but have made exceptions mostly because I don't know that we haven't
had much mind share around this.


>
> > In my mind you have unit tests, functional tests and integration tests.
> > Unit tests to me aren't designed to find bugs, or test for regressions.
> > They're specifically to test a unit of code that I've written, and the
> > tests should only depend on that unit of code that I'm intending to test.
> >
> > So for example if I write a Cinder driver called 'super_cool_driver', I
> > have a single file of unit tests that tests super_cool_driver methods
> > and NOTHING else.  It has no dependencies on any other files/modules in
> > the Cinder project.  It's only purpose is to make sure that
> > super_cool_driver's behaviours are in fact what I intended and expect.
> > It also ensures that somebody later doesn't change those behaviours
> > inadvertently.
> >
> > Testing the interaction with other Cinder modules should be done through
> > functional tests.  This is where devstack exercises would come in to
> > play.  Currently I don't think we put enough emphasis here, when
> > somebody submits a bug fix for something we often say "You need a unit
> > test to catch this sort of thing in the future", maybe that's true in
> > some cases but I'd argue that we should be saying "You need a test in
> > devstack or tempest to catch this in the future".  One of the main
> > reasons unit tests aren't very helpful here IMO is that in most cases a
> > bug results from a refactor or enhancement in the code.  Most of the
> > time when folks make these enhancements they also modify the unit test
> > to behave the way they "expect" it to and think it should.  The result
> > is they just create a unit test that verifies the incorrect behaviour.
>
> I agree that we should, across the board, suggest better tempest coverage.
>

Yeah, I'm guilty here as well but it's something I'd like to make
improvements to in H.


>
> > Another point that came up; we have "fake" volume objects and such
> > spread out and created in multiple places throughout the Cinder unit
> > tests.  Some folks use the fake class that's set up, and most create a
> > fake in their setup, or on the fly in the test case they're writing.
> > This creates a bit of a problem if the volume object ever changes (you
> > have to find every fake that's created and modify it, then modify any
> > test that uses it etc etc).  It would be great if at some point in the
> > future we had good fakes for things like volume objects, instance
> > objects etc (sort of along the lines of what Robert mentioned, but I'm
> > not sure I'm thinking of the same scope as he mentioned).
> >
> > An example of this was recently Sean Dague asked me about test objects
> > for Cinder (Volumes, Snapshots, Types etc), I was kinda surprised but we
> > didn't have an existing set of updated fakes to represent these things.
> > Seems like it would be smart to have these things (both objects and DB
> > entries) to use throughout the unit tests to provide consistency and
> > make tests a bit more realistic, rather than just rolling your own fake
> > object to fit the test your writing it for.
> >
> > Thanks,
> > John
> >
> >
> > _______________________________________________
> > OpenStack-dev mailing list
> > OpenStack-dev at lists.openstack.org
> > http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
> >
>
> _______________________________________________
> OpenStack-dev mailing list
> OpenStack-dev at lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstack.org/pipermail/openstack-dev/attachments/20130210/01d1030d/attachment.html>