3
\$\begingroup\$

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
\$\endgroup\$
2
\$\begingroup\$

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.

\$\endgroup\$
2
  • \$\begingroup\$ Thank you for the review @zachary-vance. Typo-fixes recommendations are fine. A FYI, the if fname.endswith('.pdf'): condition is used as I think that on validating the functionality with at least one PDF file must have been downloaded and the test should exit without checking for all downloaded files. I am curious about your starting phrase about brittle tests and was wondering if you would share some insights on how to write, or how would you write production grade tests? \$\endgroup\$ Oct 13 '21 at 6:21
  • \$\begingroup\$ The word brittle doesn't appear in my answer. Could be more specific? A - The original test will fail/pass when it shouldn't if: (1) Cleanup doesn't happen between tests (2) Your downloader saves the PDF with the wrong name (3) Your downloader downloads only SOME of the PDFs (4) The external website goes down B - If you match against the exact PDFs, then it will fail if (4) The external website goes down or (5) The list of PDFs changes. C - If you test against your own website, for example by running a tiny server as part of the test, then there should be no failure modes. \$\endgroup\$ Oct 18 '21 at 0:28

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.