Skip to content

Fix unittests & solr response error handling on python3 #162

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 7 commits into from
Closed

Fix unittests & solr response error handling on python3 #162

wants to merge 7 commits into from

Conversation

touilleMan
Copy link

Hi there !

Following issue #159, I went working a bit on this project

  • First I had to fix the solr installer to allow travis to run the tests
  • Then I fix issue Not 200 solr response cause crash on python3 #159 and add an unittest test about it
  • Finally, I had to mark as expected faillure test_extract given it was weirdly failing under travis

Here is the stack trace of the error (or see https://travis-ci.org/touilleMan/pysolr/jobs/71444801, the error is present both on python 2.x and 3.x)

======================================================================
ERROR: test_extract (tests.client.SolrTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "tests/client.py", line 513, in test_extract
    extracted = self.solr.extract(fake_f)
  File "pysolr.py", line 932, in extract
    files={'file': (file_obj.name, file_obj)})
  File "pysolr.py", line 331, in _send_request
    raise SolrError(error_message)
SolrError: [Reason: lazy loading error]

My guess is travis is missing a library, but I couldn't figure really what...

@elishowk
Copy link

It would be great merging and releasing a new version, downstream a django-haystack PR is blocked because of this issue https://travis-ci.org/django-haystack/django-haystack/jobs/76260939

@elishowk elishowk mentioned this pull request Aug 19, 2015
Closed
@noisy
Copy link

noisy commented Aug 20, 2015

looks ok 👍

@vinnyrose
Copy link

👍

@acdha acdha self-assigned this Sep 9, 2015
acdha
acdha reviewed Sep 24, 2015
View reviewed changes
tests/client.py
# Test bad core as well
self.solr.url = 'http://localhost:8983/solr/bad_core'
self.assertRaises(SolrError, self.solr._send_request, 'get', 'select/?q=doc&wt=json')
self.solr.url = old_url
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be a try/finally block to avoid the possibility of an unanticipated error leaving the test self.solr.url value in place

@touilleMan
Copy link
Author

I've corrected my branch with the try/finally block 👍

acdha added a commit to acdha/pysolr that referenced this pull request Sep 24, 2015
@acdha
Tests: upgrade Solr to 4.10.4
16c9934 
* Resync test Solar script with django-haystack
  These are still not quite the same; at some point it would be nice to
  look into a common tool which both projects could use
* Update Solr configuration script to set correct libpath for solr-cell
  to avoid lazy-load failures during testing as was reported on e.g. django-haystack#162
@acdha
Copy link
Collaborator

acdha commented Sep 24, 2015

The Solr errors turned out be caused by a change in file paths between Solr releases – the path in solrconfig.xml used to load the content-extraction library (solr-cell) from contrib/extraction/lib was one level off. I changed it to define the solr.install.dir property in the hopes that this will avoid similar problems in the future:

16c9934#diff-9588af6c9d46310c2f216d9e14a0b959R55

@acdha acdha closed this in cf0fa49 Sep 24, 2015
@acdha
Copy link
Collaborator

acdha commented Sep 24, 2015

After that change, the only commits left on this PR were squashed into acdha@cf0fa49 which ran successfully:

https://travis-ci.org/acdha/pysolr/builds/82057531

@touilleMan
Copy link
Author

cool 👍

The only remaining trouble is the travis status button on the README which points on a non existing target (https://travis-ci.org/toastdriven/pysolr)

@acdha
Copy link
Collaborator

acdha commented Sep 25, 2015

Yes – unfortunately, only @toastdriven can check the box to enable that CI build. I'd prefer to transfer this repo to the django-haystack organization so it doesn't depend a single person.

acdha added a commit to acdha/pysolr that referenced this pull request Feb 2, 2016
@acdha
Fix response error handling on Python 3 (closes django-haystack#162)
bf6d8b6 
Previously the error handling did not work correctly on Python 3 because
a byte-string response wasn't decoded before processing.

Thanks to Emmanuel Leblond (@touilleMan) for the patch.
@acdha
Copy link
Collaborator

acdha commented Feb 2, 2016

cf0fa49 was merged awhile ago but I just added some credits to the AUTHORS file in bf6d8b6.

If you have any testing or suggestions for the README, I'm getting ready for the v3.4.0 release.

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

@acdha acdha

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@touilleMan @elishowk @noisy @vinnyrose @acdha