Fixed Solr MLT tests #655

Closed
wants to merge 3 commits into
from

Projects

None yet

2 participants

Contributor
acdha commented Oct 1, 2012

Up to 64a192c fixes the queryset tests.

c027595 fixes the template tag tests but changes the test strategy to mocking

@toastdriven toastdriven commented on an outdated diff Oct 30, 2012
tests/solr_tests/tests/solr_backend.py
- self.assertEqual(len([result.pk for result in deferred]), 0)
-
- # Ensure that swapping the ``result_class`` works.
- self.assertTrue(isinstance(self.sqs.result_class(MockSearchResult).more_like_this(MockModel.objects.get(pk=1))[0], MockSearchResult))
+ all_mlt = self.sqs.more_like_this(MockModel.objects.get(pk=1))
+ self.assertEqual(all_mlt.count(), len([result.pk for result in all_mlt]),
+ msg="mlt SearchQuerySet .count() didn't match retrieved result length")
+
+ # Rather than hard-code assumptions about Solr's return order, we have a few very similar
+ # items which we'll confirm are included in the first 5 results. This is still ugly as we're
+ # hard-coding primary keys but it's better than breaking any time a Solr update or data
+ # change causes a score to shift slightly
+
+ # TODO: Should we care that the pks are returned as strings rather than integers? Django
+ # usually makes this transparent but it feels wrong:
+ top_results = map(int, [result.pk for result in all_mlt[:5]])
toastdriven
toastdriven Oct 30, 2012 Contributor

I'd say leave as strings for now. Please use [int(result.pk) for result in all_mlt[:5]] instead (one pass instead of two).

Contributor

Other than my one comment, LGTM. I'm +1 on more use of mock, just didn't know about it when I wrote the original tests. :shipit:

acdha added some commits Sep 30, 2012
@acdha acdha Tests: Solr MLT support
* Refactored LiveSolrMoreLikeThisTestCase into multiple tests. This
  is easier to test but will be slow until the Solr setup moves into
  a unittest2 setUpClass (or maybe even module?) method
* Refactored the MLT tests to be less brittle in checking only the
  top 5 results without respect to slight ordering variations.
* Updated MLT code to always assume deferred querysets are available
  (introduced in Django 1.1) and removed a hard-coded internal attr
  check
* Flagged what appears to be a bug in MLT with deferred querysets
83df215
@acdha acdha Tests: fix Solr MLT templatetag tests
* Updated to account for test fixture drift
* Added a random test record which should not be returned by Solr MLT queries
* Switched to unittest2 addCleanup() to ensure that our monkey-patch is always
  called
537a157
@acdha acdha Convert MLT templatetag tests to rely on mocks
This is a bit of a style change:

Advantages:
* Completely backend neutral
* Very fast
* Does not randomly break based on Solr's internal workings (e.g. run tests
  repeatedly to watch pass/fail cycles; remove data/ to be consistent)
* Avoids hard-coding fixture keys

Disadvantages:
* Relies on backend MLT queryset tests for coverage
* Mock syntax slightly obtuse

Thoughts:
* Should we simply call the template tag function directly?
b332aff
@acdha acdha closed this in 96f1624 Nov 6, 2012
@floppya floppya added a commit to floppya/django-haystack that referenced this pull request Mar 29, 2013
@acdha @floppya acdha + floppya Fix Solr more-like-this tests (closes #655)
* Refactored the MLT tests to be less brittle in checking only
  the top 5 results without respect to slight ordering
  variations.
* Refactored LiveSolrMoreLikeThisTestCase into multiple tests
* Convert MLT templatetag tests to rely on mocks for stability
  and to avoid hard-coding backend assumptions, at the expense
  of relying completely on the backend MLT queryset-level tests
  to exercise that code.
* Updated MLT code to always assume deferred querysets are
  available (introduced in Django 1.1) and removed a hard-coded
  internal attr check
1e06da8
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment