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

Ensure 'more like this' are constructor'd + fix Sunburnt tests #63

Merged
merged 3 commits into from Nov 30, 2012

Conversation

Projects
None yet
2 participants
Contributor

davidjb commented Nov 30, 2012

At present, result documents returned from Solr aren't run through the constructor as passed to a query's execute(constructor=...) method -- thus all MLT results are always plain dicts. This resolves this issue and comes with tests.

This pull request also features a bunch of test fixes to Sunburnt:

  • The tests wouldn't run originally
  • Various search tests were being missed as the good_query_data in test_search had duplicated keys
    *Implicitly tests the more-like-this functionality provided by Sunburnt
Contributor

davidjb commented Nov 30, 2012

Also, the tests related to dates within Sunburnt are failing. I would make an attempt at fixing them, but I'm unsure about how Sunburnt is expecting date/times to be stored with/without timezone information. Presumably, the dates should be stored with TZ information (if pytz is available) to clarify they are UTC, but some of the tests appear to (implicitly) test that TZ information is discarded.

Owner

tow commented Nov 30, 2012

Thanks, these all look like good fixes, especially adding the test for MLT.

There's a couple of comments on the first patch, but if those are answered, this looks good.

@davidjb davidjb commented on the diff Nov 30, 2012

sunburnt/test_search.py
([], {"boolean_field":"false"},
- {"q":u"boolean_field:true"}), # boolean field takes any truth-y value
+ [("q", u"boolean_field:false")]),
@davidjb

davidjb Nov 30, 2012

Contributor

The comment was moved above (see line 147). The reason the result was changed is that an a query is for the Boolean value 'false', so the result should be false also (not 'true' as it was) -- this test was failing.

@davidjb

davidjb Nov 30, 2012

Contributor

I also added more Boolean value testing as well (now 3 tests for false and 3 for true), hence the moving of the comment. Change this as you'd like, though.

Contributor

davidjb commented Nov 30, 2012

@tow Should be good to go now. Thanks for the comments; some inadvertent leftovers from earlier. As for the test result change as mentioned above, hopefully that explains it.

@tow tow added a commit that referenced this pull request Nov 30, 2012

@tow tow Merge pull request #63 from davidjb/mlt-constructor
Ensure 'more like this' are constructor'd + fix Sunburnt tests
7f089f8

@tow tow merged commit 7f089f8 into tow:master Nov 30, 2012

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