Add assertNoBrokenLinks() to test system

[Adrian Holovaty]

It would be convenient and useful if the test system could automatically check every <a href> in a rendered page to make sure the links did not cause 404s.

Obviously, for pages with dozens of links, this could take a long time to run. So I suggest an only_internal keyword argument, which would be True by default and would cause only the *internal* links (i.e., those without an "https?:") to be checked, thereby using Django's internal URL resolving to check the links instead of going over HTTP.

In the documentation for this feature, we should note that people should ensure none of their pages have side effects, because this test will essentially cause all links to be "clicked on."

[Michael Nelson]

Initial thoughts for feedback.

[Michael Nelson]

I've created a patch for an initial solution, but there's a few questions (see doc string for assertNoBrokenLinks()).

The regexes are pretty ugly too. I've tried to use a little indentation to make it clearer, but guessing this goes against PEP08?

Any feedback appreciated.

[Michael Radziej <mir@…>]

I haven't reviewed this thoroughly, but line 287

self.fail(u'The URL %s appears to be invalid.')

misses the final % link

[Michael Nelson]

Thanks Michael - I'll finish it today (and write the docs/tests) - was just keen to know whether the way I was going about it was ok.

[Michael Nelson]

I've added a patch with the new assertion and it's own unit tests and docs. For some reason when you click on the attachment, it seems empty, but it's there when you click on the 'raw format' option.

Let me know if there's any improvements to be made!
-Michael

[Fredrik Lundh <fredrik@…>]

Any reason you cannot just use the standard HTMLParser module? It would eliminate all those RE:s, and also get rid of the <a id="">href issue you mentioned. The sample on this page should be helpful, I think:

​http://effbot.org/librarybook/htmlparser.htm

[Adrian Holovaty]

Yes, this patch should be reworked to use HTMLParser instead of regexes. Also, please remove the commented-out print statements in the patch.

[Michael Nelson]

Thanks for the hints and the link. Will try to get the changes in today (au-time).

[Michael Nelson]

New assertNoBrokenLinks (using HTMLParser) with regression tests and docs.

[Michael Nelson]

Added new version of assertNoBrokenLinks patch using the HTMLParser, includes regression tests and docs.

I'm not sure what the process is - unchecking patch needs improvement?

[Michael Nelson]

Actually, hold off on reviewing... just thought of another test case (handling blank href's <a href=""> as the url template tag fails silently and just leaves a blank href.

Not sure if this should actually be an extra arg: ignore_blank_hrefs=False.

[Michael Nelson]

Updated patch that also checks for blank links and internal page links (ie. href="#content")

[Michael Nelson]

OK, I've uploaded a further patch that handles blank hrefs as well as verifying that internal page links (href="#content") are not broken. All with their own unit tests.

I didn't overwrite the previous patch in case you don't want the extra functionality.

[Eric Holscher]

It should be noted that I have implemented this as a third party app. It's useful in Continuous integration and as a quick sanity check of your app.

​http://github.com/ericholscher/django-crawler

I'm not sure that this should be in Django itself. It seems like it works fine as an external application, and will advance and be more useful outside of core. Setting this to 1.3 so we can make a decision about it.

[Russell Keith-Magee]

To my mind, crawling is a slightly different activity to what this test assertion is proposing.

Crawling is a process of discovering bad links; it requires that there is a tree of links from one page to another.

Link validation as a test case allows you to set up specific test conditions, and check a specific URL as an acceptance test.

Consider the case of an account activation link in an email -- there won't be a link to that URL (so a crawler shouldn't find it), but it should still be validated for invalid links. The URL for the validation link is something that should be different every time it is generated, so writing a test script to check it would be, at the very least, cumbersome.

I think there is a place for both a crawler and a link verifying test assertion.

[James Bennett]

1.3 is feature-frozen now.

[patchhammer]

assert_no_broken_links_with_tests_and_doc.2.diff fails to apply cleanly on to trunk

[Aymeric Augustin]

Change UI/UX from NULL to False.

[Unai Zalakain]

Pull request issued: ​https://github.com/django/django/pull/1740

Check for broken links in a rendered page. By default only internal links are
checked, if only_internal is False, external links are checked too.

This commit adds the patch, the tests and the docs where the possible side
effects of this test are noted.

[Tim Graham]

Someone other than the patch author needs to review and mark as RFC.

[Unai Zalakain]

Updated PR with proposed changes.

[Aymeric Augustin]

Four years later, I beg to differ with Russell. I don't believe this is sufficiently useful to be added to Django.

Django recommands to generate all URLs in templates with the {% url %}. That guarantees that URLs are valid. (Yes, I can imagine edge cases breaking this guarantee, but it holds for non-pathological URL patterns.) Regarding the "email" example, based on my experience, 95% of problems are going to be in the domain name, not in the path, so this system wouldn't be able to catch them.

Considering the ongoing efforts on Unai's pull request, I'm not going to close the ticket wontfix, but that would have been my choice otherwise.

[Unai Zalakain]

I must admit I didn't pick up this issue because I considered it particularly useful but rather because I thought it would be a funny one to work on so I beg you you don't decide whether to accept it or not on the basis of my efforts ;-)

[Russell Keith-Magee]

Replying to aaugustin:

Four years later, I beg to differ with Russell. I don't believe this is sufficiently useful to be added to Django.

Django recommands to generate all URLs in templates with the {% url %}. That guarantees that URLs are valid. (Yes, I can imagine edge cases breaking this guarantee, but it holds for non-pathological URL patterns.) Regarding the "email" example, based on my experience, 95% of problems are going to be in the domain name, not in the path, so this system wouldn't be able to catch them.

That's a good point - not sure why that didn't occur to me at the time. I'm happy to step back from my "accepted" position on this ticket.

[Tim Graham]

I'd prefer not to add this as well.