Functional tests of downloadpdf, a python CLI app

Question

Shared below is a functionality test. I would love to get some review from the community. Thank you.

The idea of the system is a simple CLI application which can be used to download PDF from a given URL. For more information please visit https://github.com/PythonCheatsheet/downloadpdf.

Test Code

#!/usr/bin/env python3
import logging
import unittest
import os
import shutil
import tracemalloc
from util.log import initilaizeLog
from util.storage import storagePath
from util.header import setHeaders
from main.scrap import scrapHREF
from main.download import downloadPDF


class TestDownloadPDF(unittest.TestCase):
    os.environ["DEBUG"] = "TRUE"
    url = ''
    folder = ''
    pdfFileExists = False

    def setUp(self):
        super(TestDownloadPDF, self).setUp()
        initilaizeLog()

    def test_shouldDownloadFromAthena(self):
        self.folder = 'athena.ecs.csus.edu'
        self.url = 'http://athena.ecs.csus.edu/~buckley/CSc191/'
        location = storagePath(self.url)
        setHeaders(self.url)
        pdfs = scrapHREF(self.url)

        self.assertIsNone(downloadPDF(self.url, location, pdfs))
        self.fileAndFolderExists(self.folder)

    def test_shouldDownloadFromWTF(self):
        self.folder = 'wtf.tw'
        self.url = 'https://wtf.tw/ref/'
        location = storagePath(self.url)
        setHeaders(self.url)
        pdfs = scrapHREF(self.url)

        self.assertIsNone(downloadPDF(self.url, location, pdfs))
        self.fileAndFolderExists(self.folder)

    def fileAndFolderExists(self, folder):
        self.assertTrue(os.path.exists('/tmp/' + folder))

        for fname in os.listdir('/tmp/' + folder):

            if fname.endswith('.pdf'):
                self.pdfFileExists = True
                break

        self.assertTrue(self.pdfFileExists)

    def tearDown(self):
        super(TestDownloadPDF, self).tearDown()

        urlPart = self.url.split("//")[1]
        folderName = urlPart.split("/")[0]
        location = '/tmp/' + folderName

        if os.path.exists(location):
            shutil.rmtree(location)

        logging.shutdown()

        self.url = ''
        self.pdfFileExists = False
        self.folder = ''

Core Logic

#!/usr/bin/env python3
import logging
import os
from urllib import request
from urllib.parse import unquote


def downloadPDF(url, storage, pdfs):

    for link in pdfs:
        decodedFileName = link.get('href')
        unquoteFileName = unquote(decodedFileName)

        request.urlretrieve(url + '/' + decodedFileName,
                            storage + '/' + unquoteFileName)

        if (os.environ.get('DEBUG')):
            break

Web Scraper

#!/usr/bin/env python3
import re
from urllib import request
from bs4 import BeautifulSoup


def scrapHREF(url):
    html = request.urlopen(url).read()
    soup = BeautifulSoup(html, features="html.parser")
    pdfs = soup.findAll("a", href=re.compile("pdf"))

    if len(pdfs) == 0:
        raise Exception('No PDFs found at ' + url)

    return pdfs

Answer 1

It's good to learn to write tests.

That said, these tests are more complicated, error-prone, and breakable by far than your actual logic, so consider whether they would be worth it in a real project. They are also longer than the code, which is perfectly normal for tests.

General Style

You have repetition. Factor this out and call it from two places:

location = storagePath(self.url)
setHeaders(self.url)
pdfs = scrapHREF(self.url)

self.assertIsNone(downloadPDF(self.url, location, pdfs))
self.fileAndFolderExists(self.folder)

Replace your custom "/tmp" logic with python's tempfile, which among other things suports deleting the directory for you.

initilaizeLog is misspelled. american english initialize is most common (brits use -ise instead). scrapHREF should be scrapeHREF and main.scap should be main.scrape. You "scape" a web page in a process called web "scaping" (scrape-ing).

Test-specific Style

You have a lot of general logic and if branches and the like. In a test, it's better to assert that exactly what you think happens, happens. Unless it's to avoid repetition, hardcode everything instead of calculating it.

The less logic you have, the better the chance that you can't mess the test up. For example, re-write

for fname in os.listdir('/tmp/' + folder):
    if fname.endswith('.pdf'):
        self.pdfFileExists = True
        break
self.assertTrue(self.pdfFileExists)

As the more specific and straightforward:

files = os.listdir('/tmp/' + folder)
self.assertEqual(files, ["a.pdf", "b.pdf"])

scrapHREF should not be done by the test and then not examined. In general, tests should not do any "real" work, they should only call functions and test the results. I would recommend breaking it into two tests:

• Test that scapHREF returns what you expect in a first test
• Use the expected result as the input to downloadPDF in a second test.

Finally, these tests depend on an external website. If that website goes down or changes, your tests will fail. This is the downside to an integration test like this when testing web scraping.