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

use pytest (bug 1090184) #369

Merged
merged 1 commit into from Dec 9, 2014
Merged

Conversation

@magopian
Copy link
Contributor

@magopian magopian commented Nov 19, 2014

This builds on top of #366 (which got rid of test_utils), and adds pytest (which is awesome).

Fixes bug 1090184

@magopian magopian force-pushed the 1090184-use-pytest branch 3 times, most recently from e68257c to dfd6a5e Nov 20, 2014
@davidbgk
Copy link
Contributor

@davidbgk davidbgk commented Nov 20, 2014

Mindblown.

@magopian magopian force-pushed the 1090184-use-pytest branch 26 times, most recently from cca1d48 to 5e50a83 Nov 22, 2014
@magopian magopian force-pushed the 1090184-use-pytest branch from 579172f to 14f30d9 Nov 25, 2014
@@ -88,6 +87,18 @@ def index(self):
tasks.index_theme_user_counts(list(user_counts))
self.refresh('stats')

def csv_to_dict(self, csv):
Copy link
Contributor

@davidbgk davidbgk Nov 26, 2014

Why don't you use anymore the csv module?

Copy link
Contributor Author

@magopian magopian Nov 26, 2014

It seemed useless. I thought about using csv.DictReader, but it needs to read from a file-like object (which is ok for response.content, but I had to trick it with StringIO or something for the "expected" string).

Do you think I still should do that?

Copy link
Contributor Author

@magopian magopian Nov 26, 2014

Actually, you're right, it makes more sense and simplifies the code ;) Thanks!

@davidbgk
Copy link
Contributor

@davidbgk davidbgk commented Nov 26, 2014

Impressive work, especially on the ES tests! r+ci

@magopian magopian force-pushed the 1090184-use-pytest branch 13 times, most recently from e195db1 to 5f22113 Nov 28, 2014

# Monkeypatch settings.LESS_BIN to not call the less compiler. We don't
# need nor want it in tests.
monkeypatch.setattr(settings, 'LESS_BIN', 'ls')
Copy link
Contributor

@davidbgk davidbgk Nov 28, 2014

Just curious, why ls?

Copy link
Contributor Author

@magopian magopian Nov 28, 2014

The real less compiler returns the name of the resulting file (which will then be added inline in the html). With ls, we also have the name of a file (the less file), which will thus be added inline in the html. We're lucky that the tests are actually grepping on a css rule that doesn't change once compiled!

@magopian magopian force-pushed the 1090184-use-pytest branch 3 times, most recently from fc64741 to ed2eae7 Nov 28, 2014
@robhudson
Copy link
Member

@robhudson robhudson commented Nov 29, 2014

I was expecting a lot more changes.

I haven't followed along much but why is pytest better? Should zamboni follow this change also?

@magopian
Copy link
Contributor Author

@magopian magopian commented Nov 29, 2014

Hello @robhudson, I answered by email ;)

The TL;DR is:

  • pytest can run nose tests, so the changes aren't that big (most of the changes are "because" of tox and its random HASHSEED
  • pytest is better for many many many reasons (see the email, or the documentation)
  • I recommend that zamboni follow this change, but we have a saying in France "les conseilleurs ne sont pas les payeurs": advisors aren't those who pay.

It did take quite some time to find all the changes needed, and the round trip didn't help (each test run used to take over 50minutes before the split in travis).
I find pytest vastly superior, but it does have a cost attached, mainly for its longer run (because it's missing the fixture bundling feature atm, and because it does have a (small) learning curve). Even if "high profile" django devs are using it, Pytest is less widespread, so there's a bigger chance a new developer already knows nose/unittest.

On the other hand, using pytest is really pythonic: simple assert statements instead of the dozen of self.assert*, or nose helpers (like eq_ or ok_).

If you plan on switching Zamboni to pytest, I'd be delighted to help!

@magopian magopian force-pushed the 1090184-use-pytest branch 2 times, most recently from f297775 to 1b6e134 Dec 8, 2014
@magopian magopian force-pushed the 1090184-use-pytest branch from 1b6e134 to 7c9c856 Dec 9, 2014
magopian added a commit that referenced this issue Dec 9, 2014
@magopian magopian merged commit 1028883 into mozilla:master Dec 9, 2014
1 check passed
@magopian magopian deleted the 1090184-use-pytest branch Dec 9, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants