Move doctests into `Makefile` and run with `test-py`

Proposal

We could (1) have our unit tests in one spot and (2) run them only once if we run pytest with --doctest-modules in the Makefile's test-py section. This feature is much desired (see, e.g., https://github.com/internetarchive/openlibrary/pull/9797#discussion_r1783633445).

Justification

No response

Breakdown

Requirements Checklist

[ ] Remove the ignores in scripts/run_doctest.sh that can be removed without causing errors. (Related files below)
[ ] For the remaining tests that don't pass if they run (hence them being commented out), update either the function or the docstring test so things are correct and --ignore isn't needed for that file.
[ ] Update test-py in Makefile to also run --doctest-modules.
[ ] Ensure docker compose run --rm web make test-py runs successfully.

Related files

scripts/run_doctests.sh
Makefile

To jump start things, here's a diff with files that it seems can have their ignore removed from run_doctests.sh:

diff --git a/scripts/run_doctests.sh b/scripts/run_doctests.sh
index 3a8434df8..e47f2b4e6 100755
--- a/scripts/run_doctests.sh
+++ b/scripts/run_doctests.sh
@@ -14,26 +14,13 @@ set -e

 pytest --doctest-modules \
         --ignore=infogami \
-        --ignore=openlibrary/catalog/marc/tests/test_get_subjects.py \
-        --ignore=openlibrary/catalog/marc/tests/test_parse.py \
-        --ignore=openlibrary/catalog/add_book/tests \
-        --ignore=openlibrary/core/ia.py \
-        --ignore=openlibrary/plugins/akismet/code.py \
-        --ignore=openlibrary/plugins/importapi/metaxml_to_json.py \
         --ignore=openlibrary/plugins/openlibrary/dev_instance.py \
-        --ignore=openlibrary/plugins/openlibrary/tests/test_home.py \
-        --ignore=openlibrary/plugins/search/code.py \
-        --ignore=openlibrary/plugins/search/collapse.py \
-        --ignore=openlibrary/plugins/search/solr_client.py \
         --ignore=openlibrary/plugins/upstream/addbook.py \
         --ignore=openlibrary/plugins/upstream/jsdef.py \
         --ignore=openlibrary/plugins/upstream/utils.py \
-        --ignore=openlibrary/records/tests/test_functions.py \
         --ignore=openlibrary/tests/catalog/test_get_ia.py \
         --ignore=openlibrary/utils/form.py \
         --ignore=openlibrary/utils/schema.py \
-        --ignore=openlibrary/utils/solr.py \
         --ignore=scripts \
-        --ignore=tests \
         --ignore=vendor \
         .

Stakeholders

@hornc

Instructions for Contributors

Please run these commands to ensure your repository is up to date before creating a new branch to work on this issue and each time after pushing code to Github, because the pre-commit bot may add commits to your PRs upstream.

@scottbarnes I'd suggest deprecating these doctests entirely. There are very few, they seem to be mostly very old helper methods, and it is unclear whether they are used or not.

These are the current doctests that run:

openlibrary/api.py s...                                                  [  0%]  Old / deprecated?
openlibrary/catalog/utils/__init__.py .                                  [  0%]  Empty file
openlibrary/core/db.py .                                                 [  1%]  single test for a python std datetime fn. More documentation than test.
openlibrary/core/helpers.py ..                                           [  1%]  simplistic documentation, don't really need testing.... one is testing python format strings
openlibrary/coverstore/archive.py .                                      [  1%]
openlibrary/coverstore/disk.py F                                         [  1%]
openlibrary/coverstore/utils.py ....                                     [  1%]
openlibrary/data/dump.py ..                                              [  3%]
openlibrary/i18n/validators.py ..                                        [ 61%]
openlibrary/olbase/events.py .                                           [ 61%]
openlibrary/plugins/books/dynlinks.py ...                                [ 62%]
openlibrary/plugins/openlibrary/code.py .                                [ 66%]
openlibrary/plugins/upstream/account.py ..                               [ 66%]
openlibrary/plugins/upstream/adapter.py ..                               [ 67%]
openlibrary/plugins/upstream/models.py .                                 [ 67%]
openlibrary/plugins/worksearch/code.py .                                 [ 70%]
openlibrary/plugins/worksearch/subjects.py s                             [ 70%]
openlibrary/plugins/worksearch/schemes/__init__.py .                     [ 70%]
openlibrary/solr/data_provider.py ..                                     [ 72%]
openlibrary/solr/query_utils.py ...                                      [ 72%]
openlibrary/solr/updater/work.py .                                       [ 72%]
openlibrary/utils/__init__.py ........                                   [ 89%]
openlibrary/utils/bulkimport.py .                                        [ 89%]
openlibrary/utils/compress.py .                                          [ 89%]
openlibrary/utils/dateutil.py ..                                         [ 90%]
openlibrary/utils/ddc.py ..                                              [ 90%]
openlibrary/utils/isbn.py .                                              [ 90%]
openlibrary/utils/lcc.py .                                               [ 90%]
openlibrary/utils/sentry.py .                                            [ 90%]

The best of them look like simple documentation examples of very general helper utils and seem to function more as documentation than tests.

Maybe these aren't a big deal, but they seem like a pain to maintain.

I've added a good chunk of these, with the intention of them being run regularly. I haven't had trouble with running these tests, what sort of issues have you hit that make them a "pain to maintain"?

@cdrini locating just the doctests was difficult, just to see what they were. To find them you have to potentially look at every comment on all methods and determine which have active code. When they pass, they are no problem, but it's difficult to get an overview of what is being tested.

For most of the ones I saw they looked so old and unmaintained my first thought was is the code this tests even used, and can we remove it?

for example, I found https://github.com/internetarchive/openlibrary/blob/50350d0432e2edf3429552bd9b50e78e662df8eb/openlibrary/coverstore/utils.py#L82C5-L88

Which looks like an ok test, but it is also tested here in test files: https://github.com/internetarchive/openlibrary/blob/50350d0432e2edf3429552bd9b50e78e662df8eb/openlibrary/coverstore/tests/test_coverstore.py#L140-L155

The test file framework at least is designed to allow for clear naming on the intent of each test, even though it is not used here.

Also, even though this method has two almost identical set of tests, it doesn't look like 'utils.urldecode()` is called from anywhere else. Having to potentially look for tests in two totally different places is an overhead.

Also, most of the one I have seen are like this:

https://github.com/internetarchive/openlibrary/blob/50350d0432e2edf3429552bd9b50e78e662df8eb/openlibrary/plugins/worksearch/code.py#L88-L94

Where as a test, this is only testing the behaviour of Python's str.split().

As documentation, is is more usefully providing an example of what the function does in the absence of a statement of the intent of the function, or why you'd want to use it.

Testing built-in functions of Python isn't useful testing, and it's not even great documentation.

I'm struggling to find one that is a good example. The best I can imagine is it documents how the method is used, but then I don't really see the value of running it every time. If there is something to test, the test should be written and located with all the other tests.

internetarchive / openlibrary