Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace develop.py with manage.py (using django-app-manage/django-better-test) #3706

Merged
merged 41 commits into from Aug 17, 2015

Conversation

ojii
Copy link
Contributor

@ojii ojii commented Dec 26, 2014

This Pull Request proposes to remove develop.py (and it's related functions in cms.test_utils) with a file called manage.py that can be used like a manage.py file in a Django project.

While develop.py works fine, it is a lot of code that has nothing to do with the actual CMS to maintain and it only supports a few commands, new commands need to be added one-by-one. django-app-manage allows to externalize that problem. It's also a project that could be used by 3rd party cms plugins, making testing easier there (this obviously doesn't require this PR to be merged, all this would do is put some weight behind it).

develop.py also set up the magic --parallel and --isolated flags for the test command, but those flags resulted in odd output. For this, I wrote django-better-test which provides those two flags, but keeps the output nice and tidy.

django-better-test also is a lot smarter about distributing the load of the tests (by keeping track of how long each test method takes), has a --failed flag to only re-run failed tests, a --retest flag to re-use the last configuration and more exciting things!

TODO:

  • add manage.py
  • remove develop.py
  • remove old code from cms.test_utils

References

https://github.com/ojii/django-app-manage
https://github.com/ojii/django-better-test

@yakky
Copy link
Member

yakky commented Dec 27, 2014

I'm definitely +1 on retiring develop.py in favor of a more flexible / modular approach.
I'm not very fond of manage.py containing all the settings (we have a ton of them) but I understand the self.contained approach
We're going to miss some abstractions, but probably we don't really need them

@ojii
Copy link
Contributor Author

ojii commented Dec 27, 2014

Well for better or worse, those settings are required to run the tests. (I literally copied them over from cms.test_utils.cli and develop.py).

The main goal here is to move boilerplate code out of the CMS so others can use it too, and it looks like we agree here. I'm open to suggestions for both app-manage and better-test, though I have to say I'm quite fond of the "explicit" way app-manage handles things (that is, explicitly define settings for your Django app to run). I wanted to go for the most minimal setup with the least magic I could come up with, and I think while there's still some magic there, I'm not sure how to get rid of it.

@yakky yakky added this to the 3.1 milestone Dec 27, 2014
@yakky
Copy link
Member

yakky commented Dec 28, 2014

@ojii while I'm not fond of having the settings in manage.py, your approach it's really the only sensible one, so I'm +1



def timed(test_labels):
return _test_run_worker(test_labels, test_runner='cms.test_utils.runners.TimedTestRunner')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will cms.test_utils.runners.TimedTestRunner be removed as well?

@yakky yakky modified the milestones: 3.1, 3.2 Mar 13, 2015
ojii added 13 commits July 15, 2015 19:34
Conflicts:
	.travis.yml
	cms/api.py
	cms/tests/test_api.py
	cms/tests/test_apphooks.py
	cms/tests/test_extensions.py
	cms/tests/test_menu_page_viewperm_staff.py
	cms/tests/test_nested_plugins.py
	cms/tests/test_rendering.py
	cms/tests/test_static_placeholder.py
	develop.py
fixed INSTALLED_APPS missing an app
Allow --use-tz flag
Fix templates config for newer Django
@mkoistinen
Copy link
Contributor

Great work, @ojii. As great as 'develop.py' had been, it was becoming an opaque hairball. Thanks for the general code cleanup along the way too. 👍

@yakky
Copy link
Member

yakky commented Aug 13, 2015

@ojii apparently there is some conflicts. Are you going to rebase this on top of develop?

@ojii
Copy link
Contributor Author

ojii commented Aug 13, 2015

do not merge, tests fail but report as passing.

@ojii
Copy link
Contributor Author

ojii commented Aug 13, 2015

Not gonna fix the 2.6 compat issues anymore until https://groups.google.com/forum/#!topic/django-cms-developers/uo9QaTw9m00 has reached a conclusion (after which ideally they won't need fixing anymore)

@mkoistinen
Copy link
Contributor

@ojii We've announced that v3.2 will be the last version to support Python 2.6. So, if this is going to get into v3.2, it needs to support Django 2.6.

@ojii
Copy link
Contributor Author

ojii commented Aug 13, 2015

fine, green now

@yakky
Copy link
Member

yakky commented Aug 14, 2015

@ojii ready for merge?

@ojii
Copy link
Contributor Author

ojii commented Aug 14, 2015

@yakky as far as I'm concerned, yes.

@yakky
Copy link
Member

yakky commented Aug 14, 2015

let's go, then
After merging this, it will be fun to forward port fixes from older versions 😄

@timgraham
Copy link
Contributor

I think ClearURLs is not needed and should be removed (at least for Django 1.7+). If it's needed, its purpose should be documented.

@ojii
Copy link
Contributor Author

ojii commented Aug 14, 2015

@yakky green again
@timgraham any more concerns?

@yakky
Copy link
Member

yakky commented Aug 15, 2015

@ojii @timgraham if no other comments on this, I'll merge it tomorrow

@ojii
Copy link
Contributor Author

ojii commented Aug 15, 2015

@yakky not that after 8 months this matters, but why wait till tomorrow?

@yakky
Copy link
Member

yakky commented Aug 15, 2015

@ojii @timgraham As I've seen so much discussion in last few hours and knowing that the interested parties are spread across a few TZs I gave a deadline comfortable enough for everyboy to comment on this, if needed

@ojii
Copy link
Contributor Author

ojii commented Aug 17, 2015

@yakky how was your weekend?

@yakky
Copy link
Member

yakky commented Aug 17, 2015

@ojii packed before my journey to zurich :( Forgive me.
Merging now :)

yakky added a commit that referenced this pull request Aug 17, 2015
Replace develop.py with manage.py (using django-app-manage/django-better-test)
@yakky yakky merged commit a022f2c into django-cms:develop Aug 17, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants