search

DjangoAdminHackers / django-linkcheck

An app that will analyze and report on links in any model that you register with it. Links can be bare (urls or image and file fields) or embedded in HTML (linkcheck handles the parsing). It's fairly easy to override methods of the Linkcheck object should you need to do anything more complicated (like generate URLs from slug fields etc).
BSD 3-Clause "New" or "Revised" License
75 stars 26 forks source link

Catch all errors in linkcheck worker, fixes #117 #119

Closed svenseeberg closed 1 year ago

svenseeberg commented 1 year ago

This PR catches all errors that may arise while calling do_check_instance_links() from the worker thread. This should hopefully prevent the worker thread from crashing as it does not do anything else.

If an error occurs, the representation of the instance and other parameters is being logged.

I have to admit that the solution is a little lazy, but I cannot think of a scenario were catching all errors would cause an issue.

This should fix #117

claudep commented 1 year ago

Thanks, now do you think you would be able to write a regression test for this?

timobrembeck commented 1 year ago

Thanks, now do you think you would be able to write a regression test for this?

I think writing a test case for this is hard because of two reasons:

  1. The queue/threading mechanism is disabled in the tests: https://github.com/DjangoAdminHackers/django-linkcheck/blob/01bbbe96391135b5621a1033d56fe3e169634630/linkcheck/listeners.py#L92-L94
  2. We would have to know an unhandled exception which we can tests for. As soon as #116 is merged, there won't be any other known bugs, so this is more like a precaution for the future I guess.
claudep commented 1 year ago

I tried to make a test, but wasn't able to push to your branch. Here it is if you want to pick it up: https://github.com/claudep/django-linkcheck/commit/dbb679880a3

timobrembeck commented 1 year ago

@claudep Ahh thanks, somehow I was so focused on checking the do_check_instance_links() function in the queue context that I didn't think about testing the standalone queue itself :sweat_smile: