Code review comment for lp://staging/~vila/ubuntu-ci-services-itself/ts-setup

Revision history for this message
Francis Ginther (fginther) wrote :

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.

« Back to merge proposal