Fix cache: Don't check one url multiple times

.travis.yml

@@ -13,9 +13,12 @@ addons:
 # command to install dependencies
 install:
   - pip install -r requirements.txt
-  - pip install pyftpdlib pytest-xdist
+  - pip install pyftpdlib pytest-xdist pytest-cov coveralls
 # command to run tests
-script: make test PYTESTOPTS="--tb=short" TESTS="tests/test_*.py tests/configuration/test_*.py tests/logger/test_*.py tests/checker/test_httpbin.py"
+script:
+  - make test PYTESTOPTS="--tb=short --cov=linkcheck" TESTS="tests/test_*.py tests/configuration/test_*.py tests/logger/test_*.py tests/cache/test_*.py tests/checker/test_httpbin.py"
 # do not send emails of broken builds
 notifications:
   email: false
+after_script:
+  - coveralls

linkcheck/cache/results.py

@@ -59,6 +59,10 @@ def has_result(self, key):
         """Non-thread-safe function for fast containment checks."""
         return key in self.cache
 
+    def has_non_empty_result(self, key):
+        """Non-thread-safe function for fast containment checks."""
+        return key in self.cache and self.cache[key]
+
     def __len__(self):
         """Get number of cached elements. This is not thread-safe and is
         likely to change before the returned value is used."""

linkcheck/cache/urlqueue.py

@@ -120,17 +120,19 @@ def _put (self, url_data):
         log.debug(LOG_CACHE, "queueing %s", url_data.url)
         key = url_data.cache_url
         cache = url_data.aggregate.result_cache
-        if url_data.has_result or cache.has_result(key):
-            self.queue.appendleft(url_data)
-        else:
-            assert key is not None, "no result for None key: %s" % url_data
-            if self.max_allowed_urls is not None:
-                self.max_allowed_urls -= 1
-            self.num_puts += 1
-            if self.num_puts >= NUM_PUTS_CLEANUP:
-                self.cleanup()
-            self.queue.append(url_data)
-        self.unfinished_tasks += 1
+        if not cache.has_result(key):
+            if url_data.has_result:
+                self.queue.appendleft(url_data)
+            else:
+                assert key is not None, "no result for None key: %s" % url_data
+                if self.max_allowed_urls is not None:
+                    self.max_allowed_urls -= 1
+                self.num_puts += 1
+                if self.num_puts >= NUM_PUTS_CLEANUP:
+                    self.cleanup()
+                self.queue.append(url_data)
+            self.unfinished_tasks += 1
+            cache.add_result(key, None) # add none value to cache to prevent checking this url multiple times
 
     def cleanup(self):
         """Move cached elements to top."""
@@ -139,7 +141,7 @@ def cleanup(self):
         for i, url_data in enumerate(self.queue):
             key = url_data.cache_url
             cache = url_data.aggregate.result_cache
-            if cache.has_result(key):
+            if cache.has_non_empty_result(key):
                 cached.append(i)
         for pos in cached:
             self._move_to_top(pos)

linkcheck/configuration/__init__.py

@@ -81,7 +81,7 @@ def get_modules_info():
     for (mod, name, version_attr) in Modules:
         if not fileutil.has_module(mod):
             continue
-        if hasattr(mod, version_attr):
+        if version_attr and hasattr(mod, version_attr):
             attr = getattr(mod, version_attr)
             version = attr() if callable(attr) else attr
             module_infos.append("%s %s" % (name, version))

tests/cache/test_urlqueue.py

@@ -0,0 +1,174 @@
+# -*- coding: utf-8 -*-
+# Copyright (C) 2017 Petr Dlouhý
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License along
+# with this program; if not, write to the Free Software Foundation, Inc.,
+# 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+
+import unittest
+from collections import namedtuple
+
+from linkcheck.cache.results import ResultCache
+from linkcheck.cache.urlqueue import Empty, NUM_PUTS_CLEANUP, UrlQueue
+
+UrlData = namedtuple('UrlData', 'url cache_url aggregate has_result')
+Aggregate = namedtuple('Aggregate', 'result_cache')
+
+
+class TestUrlQueue(unittest.TestCase):
+
+    def setUp(self):
+        self.result_cache = ResultCache()
+        self.urlqueue = UrlQueue()
+        self.urldata1 = UrlData(
+            url="Foo",
+            cache_url="Foo",
+            aggregate=Aggregate(
+                result_cache=self.result_cache,
+            ),
+            has_result=True,
+        )
+
+    def test_max_allowed_urls_bad_value(self):
+        with self.assertRaises(ValueError):
+            UrlQueue(max_allowed_urls=0)
+        with self.assertRaises(ValueError):
+            UrlQueue(max_allowed_urls=-1)
+
+    def test_qsize(self):
+        """ Test qsize() """
+        self.assertEquals(self.urlqueue.qsize(), 0)
+        self.urlqueue.put(self.urldata1)
+        self.assertEquals(self.urlqueue.qsize(), 1)
+
+    def test_empty(self):
+        """ Test empty() """
+        self.assertEquals(self.urlqueue.empty(), True)
+        self.urlqueue.put(self.urldata1)
+        self.assertEquals(self.urlqueue.empty(), False)
+
+    def test_get_empty(self):
+        """ Test, that get() with empty queue throws Empty """
+        with self.assertRaises(Empty):
+            self.assertEquals(self.urlqueue.get(0.01), None)
+
+    def test_get_negative_timeout(self):
+        """ Test, that get() with negative timeout throws ValueError """
+        with self.assertRaises(ValueError):
+            self.assertEquals(self.urlqueue.get(-1), None)
+
+    def test_put_get(self):
+        """
+        Test, that after put() we can get()
+        the item and it can be get only once
+        """
+        self.urlqueue.put(self.urldata1)
+        cached_item = (
+            self.urldata1.aggregate.result_cache.get_result(self.urldata1)
+        )
+        self.assertEquals(cached_item, None)
+        self.assertEquals(self.urlqueue.get(), self.urldata1)
+        with self.assertRaises(Empty):
+            self.assertEquals(self.urlqueue.get(0.01), None)
+
+    def test_put_has_result_false(self):
+        """
+        Test, that element with has_result=False
+        is put() on the end of queue
+        """
+        self.urlqueue.put(self.urldata1)
+        urldata = UrlData(
+            url="Bar",
+            cache_url="Bar",
+            aggregate=Aggregate(
+                result_cache=self.result_cache,
+            ),
+            has_result=False,
+        )
+        self.urlqueue.put(urldata)
+        self.assertEquals(self.urlqueue.get(), self.urldata1)
+        self.assertEquals(self.urlqueue.get(), urldata)
+        with self.assertRaises(Empty):
+            self.assertEquals(self.urlqueue.get(0.01), None)
+
+    def test_put_has_result_true(self):
+        """
+        Test, that element with has_result=True
+        is put() on the beginning of queue
+        """
+        self.urlqueue.put(self.urldata1)
+        urldata = UrlData(
+            url="Bar",
+            cache_url="Bar",
+            aggregate=Aggregate(
+                result_cache=self.result_cache,
+            ),
+            has_result=True,
+        )
+        self.urlqueue.put(urldata)
+        self.assertEquals(self.urlqueue.get(), urldata)
+        self.assertEquals(self.urlqueue.get(), self.urldata1)
+        with self.assertRaises(Empty):
+            self.assertEquals(self.urlqueue.get(0.01), None)
+
+    def test_put_cache(self):
+        """
+        Test, that making put() on two elements with same
+        cache_url adds only one element
+        """
+        self.urlqueue.put(self.urldata1)
+        urldata = UrlData(
+            url="Bar",
+            cache_url="Foo",
+            aggregate=Aggregate(
+                result_cache=self.result_cache,
+            ),
+            has_result=True,
+        )
+        self.urlqueue.put(urldata)
+        self.assertEquals(self.urlqueue.qsize(), 1)
+        self.assertEquals(self.urlqueue.get(), self.urldata1)
+        with self.assertRaises(Empty):
+            self.assertEquals(self.urlqueue.get(0.01), None)
+
+    def test_cleanup(self):
+        """
+        Test, that after adding NUM_PUTS_CLEANUP elements
+        the queue is cleaned up.
+        Whether the cleanup is was performed is determined,
+        that element in cache is now on top of the queue.
+        """
+        for i in range(NUM_PUTS_CLEANUP - 1):
+            self.urlqueue.put(
+                UrlData(
+                    url="Bar",
+                    cache_url="Bar address %s" % i,
+                    aggregate=Aggregate(
+                        result_cache=self.result_cache,
+                    ),
+                    has_result=False,
+                ),
+            )
+        self.assertEquals(self.urlqueue.qsize(), NUM_PUTS_CLEANUP - 1)
+        urldata = UrlData(
+            url="Bar",
+            cache_url="Bar address",
+            aggregate=Aggregate(
+                result_cache=self.result_cache,
+            ),
+            has_result=False,
+        )
+        self.result_cache.add_result("Bar address 2", "asdf")
+        self.urlqueue.put(urldata)
+        self.assertEquals(self.urlqueue.qsize(), NUM_PUTS_CLEANUP)
+        self.assertEquals(self.urlqueue.get().cache_url, "Bar address 2")