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

Already on GitHub? Sign in to your account

Test fixes #487

Closed
wants to merge 5 commits into
from

Conversation

Projects
None yet
5 participants
Contributor

almet commented Oct 30, 2012

These are different commits that fix some Elastic-Search related tests.

All of them are not fixed yet, because we have some leaks from one test to another (tests not cleaning after themselves).

There is a particular change I would need to have reviewed: the removal of the search for author prefixes, when doing a search. I didn't managed to understand why this was failing, but removing this bit made the tests pass; so I'm not sure this is the fix we want.

almet added some commits Oct 22, 2012

@almet almet Fix mkt.browse tests
The category page needs either 3 or 0 featured apps.

Also make a little refactor on how the creation of featured apps is handledin the tests.
4206bfe
@almet almet Remove a test that's not valid anymore. a948c21
@almet almet Refactor featured applications testing.
The "setup_featured" method nows accepts the number of items that it should
generates, and doesn not return anything anymore.

Instead, it stores the applications in the context of the test object.

Update the tests to use this new method signature.
4ddb9f8
@almet almet General enhancements on variable naming in tests. 1304745

@mattbasta mattbasta commented on the diff Oct 30, 2012

mkt/browse/tests/test_views.py
+ desktop=False):
+ if mobile and desktop:
+ raise ValueError('an app could not be only desktop and mobile')
+ # Home featured.
+ app = amo.tests.app_factory()
+ self.make_featured(app=app, category=category)
+
+ # Make this app compatible on only desktop.
+ if desktop:
+ app.addondevicetype_set.create(device_type=amo.DEVICE_DESKTOP.id)
+ if mobile:
+ app.addondevicetype_set.create(device_type=amo.DEVICE_MOBILE.id)
+ return app
+
+ def setup_featured(self, nb_category=3, nb_home_desktop=1,
+ nb_home_mobile=0):
@mattbasta

mattbasta Oct 30, 2012

Contributor

Since nb_home_mobile is only used in mkt/home, do you think it would be better to override setup_featured() in TestHome rather than having it in here?

@mattbasta

mattbasta Oct 30, 2012

Contributor

The same could probably go for _create_featured_app(): if we're not using the code where we're writing it, it becomes difficult to determine where or whether it's used in the future.

@almet

almet Nov 13, 2012

Contributor

I completely agree with this; I just re-factored without knowing exactly where it was used.

@mattbasta mattbasta commented on the diff Oct 30, 2012

mkt/browse/tests/test_views.py
@@ -173,12 +209,13 @@ def test_featured_region_exclusions(self):
self._test_featured_region_exclusions()
def test_featured_src(self):
- _, _, featured = self.setup_featured()
+ self.setup_featured()
+ featured = self.home_desktop_featured[0]
@mattbasta

mattbasta Oct 30, 2012

Contributor

I like this so much better.

@mattbasta mattbasta commented on an outdated diff Oct 30, 2012

mkt/browse/tests/test_views.py
r = self.client.get(self.url)
doc = pq(r.content)
- featured_tiles = doc('.featured .mkt-tile')
- eq_(featured_tiles.eq(0).attr('href'),
- a.get_detail_url() + '?src=mkt-category-featured')
- eq_(featured_tiles.eq(1).attr('href'),
- b.get_detail_url() + '?src=mkt-category-featured')
+ featured_tiles = doc('#featured .mkt-tile')
+
+ for i in range(2):
@mattbasta

mattbasta Oct 30, 2012

Contributor

Is there a reason you can't test all of them (if they exist)? What happens if (by some dark magic) there are three instead of two?

for tile, listing in zip(featured_tiles, featured):

@mattbasta mattbasta commented on an outdated diff Oct 30, 2012

mkt/home/tests/test_views.py
+ # create 5 featured apps on the homepage
worldwide_apps = [app_factory().id for x in xrange(5)]
@mattbasta

mattbasta Oct 30, 2012

Contributor

If you're going to use xrange(), you'd might as well make this a generator expression.

@mattbasta mattbasta commented on an outdated diff Oct 30, 2012

mkt/search/tests/test_views.py
@@ -566,17 +570,17 @@ def test_url(self, url, app_is_premium=True, device_is_gaia=False):
url = urlparams(url, gaia='true' if device_is_gaia else 'false')
self.refresh()
- r = self.client.get(url, follow=True)
- eq_(r.status_code, 200)
+ resp = self.client.get(url, follow=True)
@mattbasta

mattbasta Oct 30, 2012

Contributor

pdb thanks you

@mattbasta mattbasta commented on the diff Oct 30, 2012

mkt/search/tests/test_views.py
def test_generator(self):
# This is necessary until we can get test generator methods worked out
# to run properly.
- for test_params in self._generate():
- func, params = test_params[0], test_params[1:]
- func(*params)
+ for params in self._generate():
@mattbasta

mattbasta Oct 30, 2012

Contributor

This was my code and I apologize for this. Thanks for making it better.

@mattbasta mattbasta commented on an outdated diff Oct 30, 2012

mkt/browse/tests/test_views.py
@@ -40,30 +44,60 @@ def make_featured(self, app, category=None):
region=mkt.regions.US.id)
return f
- def setup_featured(self, num=3):
+ def _create_featured_app(self, category=None, mobile=False,
+ desktop=False):
+ if mobile and desktop:
@mattbasta

mattbasta Oct 30, 2012

Contributor

You could probably just do an assertion here for brevity. The test(s) will fail either way.

assert not (mobile and desktop), 'An app could not be only desktop and mobile.'

stuff is confusing, I know

can you align this with the "s" in "self" above?

this is smarter, thanks

although this is an improvement, I still don't like how this works. it's very hard to follow - and that's 100% my fault.

why two new newlines?

shouldn't this be eq(i) and featured[i]?

yeah, we still want to have this. but ES was having issues. and I can't find where the code went - I thought I had just commented it out.

ah, good. better.

you fixed here, I see

# Comments should be full sentences with periods.

if we're going to change this, we should go with res since that's the second-most-common name used in zamboni for test responses

Contributor

almet commented Nov 13, 2012

Too much mail, I failed to see you commenting here earlier, @mattbasta; I'll update the PR later today or tomorrow.

Owner

clouserw commented Jan 29, 2013

Alexis: what's the status of this? Still something that needs review or something else?

Member

cvan commented Jan 29, 2013

I forgot this pull request existed. I've since fixed the home and browse tests in 813666b

Owner

hannosch commented Mar 19, 2013

As of today all non-skipped ES tests actually pass. So this pull request has been superseded by lots of other changes.

@hannosch hannosch closed this Mar 19, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment