NYPL-Simplified / server_core

Shared data model and utilities for Library Simplified server applications
7 stars 11 forks source link

Replace nosetests with pytest #1245

Closed jonathangreen closed 3 years ago

jonathangreen commented 3 years ago

Overview

In anticipation of the work to move from Python 2 to Python 3 I want to make sure that the CI system in place can smoothly handle this transition. During the initial testing with Python 3 we had some issues with the nose log capture handler.

Looking into nosetests, I noticed it had its last update in 2016 and has recommended that it not be used for new projects since 2015.

Nose has been in maintenance mode for the past several years and will likely cease without a new person/team to take over maintainership. New projects should consider using Nose2, py.test, or just plain unittest/unittest2.

Work

Pytest seems widely used and supported, and has some support for running nose style tests. This PR includes all the work to replace nosetests with pytest and to run the new pytest tests with github actions rather then Travis CI.

There are a lot of changes here so I've tried to break them out into commits that might make reviewing these changes easier. Each one has a fairly descriptive commit message.

There are 2 broad categories of work here:

Testing

If you have all the services used by the tests setup you can simply run the tox command and it will run the tests.

To test with all the services in docker containers:

# Start the containers for testing
docker run -d -p 9005:5432/tcp --name db -e POSTGRES_USER=simplified_test -e POSTGRES_PASSWORD=test -e POSTGRES_DB=simplified_circulation_test postgres:9.6
docker run -d -p 9006:9200/tcp --name es -e discovery.type=single-node elasticsearch:6.8.6
docker run -d -p 9007:9000/tcp --name minio -e MINIO_ACCESS_KEY=simplified -e MINIO_SECRET_KEY=12345678901234567890 bitnami/minio:latest

# Add elasticsearch plugin
docker exec es elasticsearch-plugin install -s analysis-icu
docker restart es

# Set environment variables
export SIMPLIFIED_TEST_DATABASE="postgres://simplified_test:test@localhost:9005/simplified_circulation_test"
export SIMPLIFIED_TEST_ELASTICSEARCH="http://localhost:9006"
export SIMPLIFIED_TEST_MINIO_ENDPOINT_URL="http://localhost:9007"
export SIMPLIFIED_TEST_MINIO_USER="simplified"
export SIMPLIFIED_TEST_MINIO_PASSWORD="12345678901234567890"

# Run tox
tox

Additional Work

There is some open work to do before this PR could be merged:

I wanted to get this initial work wrapped up in a PR to allow some time to review / discuss the changes before those follow up PRs. @leonardr @tdilauro @vbessonov review or feedback about the approach here would be great.

leonardr commented 3 years ago

FYI @EdwinGuzman has a Python 3 port branch in progress here: https://github.com/NYPL-Simplified/server_core/pull/1244. He hasn't moved from nosetests to pytest yet but I know that's something he and @nballenger were planning to work on.

Since this isnt' directly related to py3 I think the best strategy is to merge this into develop and then merge develop into Edwin's feature branch.

leonardr commented 3 years ago

Having gone through this enormous diff I agree that this is the way forward. I'd like to merge this after these two changes:

jonathangreen commented 3 years ago

Re: changes to the status checks. I think actions would run on this branch if it were coming from inside of the repo. If you want to push this branch into the NYPL repo, or give me access to, we could test that theory. Then we can see tests run though CI and pass.

leonardr commented 3 years ago

You're right. I copied your code into a branch on the server_core repo and the checks do run: https://github.com/NYPL-Simplified/server_core/pull/1246/checks?check_run_id=2086702626#step:6:58

There are a lot of test failures, because the SIMPLIFIED_TEST_DATABASE environment variable isn't set at the point when the checks are run. If you can suggest a fix for that and push it to this branch, I'll mirror it and try again.

I'll use https://github.com/NYPL-Simplified/server_core/pull/1246 to test things out, but I'll do a final merge of this branch to preserve credit and revision history.

jonathangreen commented 3 years ago

@leonardr I think there are a few files missing from what you pushed to #1246 and that is why its failing.

I believe you should be able to push the same commits from this PR into a branch on this repository like this (this assumes your origin remote is NYPL-simplified):

# Remove local pytest branch
git branch -D pytest
# Pull this PR into a branch called pytest
git fetch origin pull/1245/head:pytest
# Push it up to the NYPL repo, overwriting the existing pytest branch
git push --force origin pytest
leonardr commented 3 years ago

Oh, that's great -- I didn't know about the syntax for fetching from a PR. Now all the tests run with a single failure: https://github.com/NYPL-Simplified/server_core/runs/2089258674?check_suite_focus=true

jonathangreen commented 3 years ago

It looks like actions are now running on this branch. 🎉 And the tests are passing so that is a win.

I brought this up to date with the latest commits to develop and pushed in a few other changes that were necessary in getting pytest running the circulation tests.

leonardr commented 3 years ago

After talking it over with Edwin (to coordinate with his Python 3 work) I've disabled the Travis integration for server_core and this branch is good to merge. Long live pytest!