Merge lp://staging/~vila/ubuntu-ci-services-itself/ts-setup into lp://staging/ubuntu-ci-services-itself

PASSED: Continuous integration, rev:221
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/118/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/118/rebuild

ticket_system/ticket_system/local_tests.py is for running manage.py test.. Please don't remove that.

> ticket_system/ticket_system/local_tests.py is for running manage.py test..
> Please don't remove that.

How many commands are used to run tests ? Where are they documented ?

> ticket_system/ticket_system/local_tests.py is for running manage.py test..
> Please don't remove that.

Moreover, setup.py test has been fixed and already calls 'manage.py test' with the right arguments (the local apps unless you provide something more specific) so I don't get what you're objecting to, please elaborate.

https://docs.djangoproject.com/en/1.6/topics/testing/overview/#running-tests

As manage.py test is something that comes with django, I would prefer to leave this so that someone who is coming in to the project can keep the normal django development ways without having to figure out that setup.py 'does the right thing'

> https://docs.djangoproject.com/en/1.6/topics/testing/overview/#running-tests

Good, this describes how a newcomer should run tests when he joins a new project that is a single django app. A single command to run the tests.

> As manage.py test is something that comes with django, I would prefer to leave
> this so that someone who is coming in to the project can keep the normal
> django development ways without having to figure out that setup.py 'does the
> right thing'

Then apply the same logic to the usci project itself, this MP is a step along the path to provide a single command to run all the tests in the project.

A newcomer should not have to learn a different command for every component of the project, plus one for the integration tests, plus several others for tests that doesn't fit in these categories.

Also note that the page you're referring to is for django 1.6 and I note that they say:
 Changed in Django 1.6:

Previously, test labels were in the form applabel, applabel.TestCase, or applabel.TestCase.test_method, rather than being true Python dotted paths, and tests could only be found within tests.py or models.py files within a Python package listed in INSTALLED_APPS. The --pattern option and file paths as test labels are new in 1.6.

So even in django the way you run tests change, so I'd rather maintain a single place to define, select and run tests for the whole project than having to maintain a different code base for each component.

I'm still -1.. There are quite a number of Django devs in the community.. If one of them decides to work on the ticket system at some point, they would have to learn this new command instead of just doing the manage.py test that they are familiar with.

On 02/10/2014 01:29 PM, Vincent Ladeuil wrote:
> A newcomer should not have to learn a different command for every
> component of the project, plus one for the integration tests, plus
> several others for tests that doesn't fit in these categories.

Is it that odd? Django guys run things one way, standard python guys
another. I think its odd we are trying to create a standard:

   https://xkcd.com/927/

> I'd rather
> maintain a single place to define, select and run tests for the whole
> project than having to maintain a different code base for each
> component.

I don't agree this problem is as bad as you say it is. As a python and
django developer I expect a couple of things:

  1) i can run tests via ./setup.py test
  2a) I can test python code with: python -m unittest <discover|<path>>
  2b) I can test python-django with ./manage.py test

We currently support all of this. In addition we have a handy script,
tarmac.sh which can run all unit tests as well as tests/run.py which can
run all integration tests if you have a cloud.

I agree with Chris. I don't see any need to remove the ability to run tests the way that those of us on the team who've been doing a lot of django development are used to.

I think the point of discussion here is that removing ticket_system/ticket_system/local_tests.py (and the associated change in ticket_system/ticket_system/settings.py) changes the behavior of './manage tests'. Before this MP, './manage tests' only runs the tests under settings.LOCAL_APPS, with this change './manage tests' runs these plus a slew of django tests that are unrelated to this project (and takes 10x run time on my machine). To me, './manage tests' is a convenience for those heavily involved in this component and django devs in general, I see no need to change this behavior.

The bigger concern to me is that we do need one single way to run the tests (the canonical way :-) ) so that I don't have to think about how to run the tests when going from component to component. The change to ticket_system/setup.py does this as it makes sure all apps are executed, not just the specified ones. Now I can run ./setup test and the right thing will just happen. Furthermore, this is also what tarmac.sh is doing, our changes are ultimately verified against 'the one true way'.

In summary, the recommendation is to keep the change to ticket_system/setup.py, but revert the remaining changes.

PASSED: Continuous integration, rev:222
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/135/
Executed test runs:

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/uci-engine-ci/135/rebuild

The overall reaction to that MP makes me sad.

People seem to be fighting to keep a specific test runner whose only purpose
is to restrict the set of tests that are run...

Would everybody, please, have a look at what is at stake here and how we are
currently managing it:

- django (by default) run all tests for the code that is involved in a
  django app,
- people don't want to run those all those tests all the time,
- restricting the tests to only the local apps is implemented in two
  different ways.

*Not* running the tests for the apps we're relying upon is a risk. I could
argue that this is a bad practice and what is needed instead is a way to run
them only in specific cases (once a day ?). But that's out of scope for this
MP.

You're arguing that your selection of tests is the '1' true way and should
be kept at all costs.

I'm arguing that this is too coarse and that we need a far finer grain when
selecting tests for and that we should have to maintain two places to handle
that.

Finally, the ticket system is only one part of the project, nobody should
think that running only the tests for that component is good enough, it
should still integrate properly into the project and the way to ensure that
is to run all the tests for the whole project.

I don't understand why people are objecting to steps that get us closer to
that goal.

Anyway, I've fixed manage.py to do what most people expect when using
manage.py but the specific test runner is still not needed.

Now with the objections:

> I'm still -1.. There are quite a number of Django devs in the
> community.. If one of them decides to work on the ticket system at some
> point, they would have to learn this new command instead of just doing the
> manage.py test that they are familiar with.

That's not accurate. A newcomer that want to run the tests for the ticket
system does not simply run './manage.py test', it first need to setup the
right env for that, which requires running (at least AFAIK and that's
exactly why I want to fix that because I still have to know far too much):

  virtualenv /whatever
  . /whatever/bin/activate
  ./ci-utils/setup.py develop
  ./ticket_system/setup.py develop
  cd ticket_system
  ./manage.py test

Where will they learn that ? And even if there was a proper documentation on
how to run the tests for the ticket system, if we don't have a single
command to run the tests for the whole project, my point above about running
the relevant tests still apply.

   https://xkcd.com/927/

That would be true if we were targeting feature parity, that's not the case,
I want a newcomer to be able to run a single command to run the tests for
the whole project.

 > I don't agree this problem is as bad as you say it is. As a python and
 > django developer I expect a couple of things:
 >
 > 1) i can run tests via ./setup.py test
 > 2a) I can test python code with: python -m unittest <discover|<path>>
 > 2b) I can test python-django with ./manage.py test

Meh, none of that run out of the box, see above.

Granted, run-tests doesn't setup the env yet either, but that's still the
plan.

I find it ironic that you're pointing that xkcd link while at the same time
remindin...

We aren't changing django, south, or tastypie during our development. Why would we want to run their tests?

I don't think anyone is opposed to having *a* way to run all of the tests for the entire ci engine.

I personally do not want to run all the tests very often but when I'm working on a component I want an easy way to test just that component I'm working on, perhaps even just one small piece of that component. I don't want to have to run:

  ../run-tests <some obscure incantation to get me down to a single testcase or suite>

which is ultimately calling:

  ./manage.py test <app>.TestCaseClass.test

It looks to me like the changes on L21-24 remove the ability to specify tests from setup.py as well. What is the reasoning behind this?

As for changing manage.py, that file is generated by django in the process of setting up a project. I avoid changing it.

Lastly, I find your commit message offensive and condescending.

What of this is actually needed to make your 'run-tests' work, or does it work as is, you just want to make these changes?

Sorry.. Should have been needs info, not needs fixing. Wrong click.

On 02/11/2014 02:51 AM, Vincent Ladeuil wrote:
> tests/run.py is not integrated in tarmac.sh and none of them provide a way
> to select subsets of the test suite.

The threads have gotten nuts, but I think this is a key point of yours
and might be this disconnect. I think some people prefer to have the
unit-tests and integration tests be separate and have 2 commands to
launch them.

So maybe as general discussion point we should decide: do we want a
single "all tests" command or a "unit-tests" command (implies something
runs really fast) and an "integration tests" command (implies something
a bit slower)?

NOTE: test/run.py does let you specify which suites to run.

> We aren't changing django, south, or tastypie during our development. Why
> would we want to run their tests?

To catch regressions ? Why do you think the django devs did that behavior
the default one ?

>
> I don't think anyone is opposed to having *a* way to run all of the tests for
> the entire ci engine.
>
> I personally do not want to run all the tests very often but when I'm working
> on a component I want an easy way to test just that component I'm working on,
> perhaps even just one small piece of that component. I don't want to have to
> run:
>
> ../run-tests <some obscure incantation to get me down to a single testcase
> or suite>
>
> which is ultimately calling:
>
> ./manage.py test <app>.TestCaseClass.test
>

As it happens that obscure incantation is:

  ./run-tests <app>.TestCaseClass.test

what's obscure with that ?

As an added bonus, if your test has a unique method name you can even just
do:

  ./run-tests test_unique_name

or whatever regexp that captures your need without having to switch to a
different syntax when you work on a different component.

> It looks to me like the changes on L21-24 remove the ability to specify tests
> from setup.py as well. What is the reasoning behind this?

That setup.py calls manage.py ?

>
> As for changing manage.py, that file is generated by django in the process of
> setting up a project. I avoid changing it.

It has already been changed.

> Lastly, I find your commit message offensive and condescending.

Then I'm all ears about why a technical change that address your needs raise
such reactions ?

'obscure incantation' is also offensive when I'm offering a simpler
aproach. I can live with that as long as progress is made.

> What of this is actually needed to make your 'run-tests' work, or does it work
> as is, you just want to make these changes?

As said in the cover letter:

Drive-by fix from another branch I'm working on.

ticket_system/setup.py for the 'test' command default to injecting the list of local apps. Since that's already defined in settings.LOCAL_APPS, just use that.

Since the above was duplicating ticket_system/ticket_system/local_tests.py,
it's not needed anymore, I removed it.

I.e. the changes are unrelated, this proposal just simplify the way setup/manage restrict the tests run.

On Tue, Feb 11, 2014 at 11:00 AM, Andy Doan <email address hidden>wrote:

>
> So maybe as general discussion point we should decide: do we want a
> single "all tests" command or a "unit-tests" command (implies something
> runs really fast) and an "integration tests" command (implies something
> a bit slower)?
>
> NOTE: test/run.py does let you specify which suites to run.
>
>
Yes.. There does need to be two separate commands.. Running the integration
tests all the time right now is not a good idea for many reasons. They
should be run at a fixed interval and reported on, but again, I don't know
that we have an environment worth doing this on yet. I'm still running them
a few times a day manually and report any issues I see.

Based upon the fact that this removes functionality, we don't need it and we don't have time to play with and implement our own test runner, -1.

i don't know if we should totally scrap this. One cool idea from this branch was using settings.LOCAL_APPS instead of hard-coding the apps in setup.py. how it was done (by changing manage.py) isn't right. I'd say we can iterate off this, or just do a new MP with that simple change.

On Tue, Feb 18, 2014 at 5:21 PM, Andy Doan <email address hidden>wrote:

> i don't know if we should totally scrap this. One cool idea from this
> branch was using settings.LOCAL_APPS instead of hard-coding the apps in
> setup.py. how it was done (by changing manage.py) isn't right. I'd say we
> can iterate off this, or just do a new MP with that simple change.
>

I would be ok with an MP using settings.LOCAL_APPS... at the present time,
I'm still -1 for the MP as it stands.