Simplify test database setup

[kalbfled]

User Story - Business Need

• Ticket is understood, and QA has been contacted (if the ticket has a QA label).

User Story(ies)

As a Notify developer,
I want the unit tests to create one test database, rather than one for each Pytest thread,
So that to simplify the code and facilitate future test improvements.

Additional Info and Resources

As currently implemented, running unit tests results in the creation of one test database for each Pytest thread, and each database name has an appended worker ID to name is something like "test_notification_api_gw0".

Databases have worker threads, so the more complicated multi-db setup doesn't improve performance. Given our desire to pre-seed the test database and restore proper rollback functionality after each unit test (so tests are always repeatable), we desire a more simple, single database setup.

Engineering Checklist

• Remove all uses of worker_id. You can find them by running grep -rni worker_id .. This includes work in the VA Profile integration lambda.
• After you do the above checkbox, instead of trying to fix any failing tests, please reach out to the team to inform them that tests are failing. The team will determine next steps together. (This is pointed at a 5 with this checklist item considered. Please mind the scope.) --> Everything fails; re-pointed to many more points.
• Fix all failing tests
• Delete all unused fixtures in tests/conftest.py and tests/app/conftest.py
• Delete all unused "create_" functions in tests/app/db.py

This might be all that is necessary. The test fixture notify_db calls create_test_db, which should not recreate a database that already exists.

Acceptance Criteria

• No significant slow down of unit tests noted
• After running unit tests, the only test database in the container is "test_notification_api".

Out of Scope

This is the first ticket in an epic. Future tickets will:

• Restore proper rollback functionality
• Pre-seed the test database
• After the above verifications, delete the test database after it runs. See here.

[mjones-oddball]

@kalbfled We should remove the optional line below and just beef up this old todo ticket for that topic: https://app.zenhub.com/workspaces/va-notify-620d21369d810a00146ed9c8/issues/gh/department-of-veterans-affairs/notification-api/900
Can you add that to your todo list for next week?

Optional: After the above verifications, consider another revision to delete the test database after it runs. See here.

[mjones-oddball]

Hey team! Please add your planning poker estimate with Zenhub @cris-oddball @EvanParish @kalbfled @ldraney @nikolai-efimov

[kalbfled]

Having one database for all test threads causes a multitude of IntegrityError exceptions. I suspect that this is due to tests creating instances with hard-coded IDs and other values.

[kalbfled]

The IntegrityError exceptions were occurring during test setup, and they are fairly straight-forward to eliminate by not using hard-coded values for model IDs and other unique attributes. However, fixing the setup errors exposes the teardown errors. Most tests use the fixture notify_db_session, which includes the teardown step of dropping tables. This is not acceptable for a single database test setup.

I think the solution is to restore usage of the pytest-flask-sqlalchemy add-on, which we removed during the Flask upgrade last April. The configuration and usage seems easy enough, but I will have to refactor every test that uses the notify_db or notify_db_session fixture.

[npmartin-oddball]

Rationale for point estimate modification from 5 to 8, as well as "Off-track" label detailed above in Dave's comment.

[mjones-oddball]

Dave:

notification-api#1436 does not have a quick resolution. Fixing the test setup errors is simple, but the teardown is a bigger issue. The existing code drops tables, but that is inappropriate with one test database. I'm trying to restore the rollback functionality that existing before our Flask upgrade last April. I can spend the rest of the sprint on this.

[kalbfled]

I ran into this issue. pytest-flask-sqlalchemy has not had any new commits since April 2022 and has breaks when used with flask-sqlalchemy >= 3.0.

request = <SubRequest '_transaction' for <Function test_get_communication_item>>, _db = <SQLAlchemy postgresql://postgres:LocalPassword@db:5432/test_notification_api +1>
mocker = <pytest_mock.plugin.MockerFixture object at 0x7f9998b02790>

    @pytest.fixture(scope='function')
    def _transaction(request, _db, mocker):
        '''
        Create a transactional context for tests to run in.
        '''
        # Start a transaction
        connection = _db.engine.connect()
        transaction = connection.begin()
    
        # Bind a session to the transaction. The empty `binds` dict is necessary
        # when specifying a `bind` option, or else Flask-SQLAlchemy won't scope
        # the connection properly
        options = dict(bind=connection, binds={})
>      session = _db.create_scoped_session(options=options)

/usr/local/lib/python3.8/site-packages/pytest_flask_sqlalchemy/fixtures.py:38: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <SQLAlchemy postgresql://postgres:LocalPassword@db:5432/test_notification_api +1>, name = 'create_scoped_session'

    def __getattr__(self, name: str) -> t.Any:
...
>       raise AttributeError(name)
E       AttributeError: create_scoped_session

/usr/local/lib/python3.8/site-packages/flask_sqlalchemy/extension.py:1008: AttributeError

I'm looking for a work-around. Here is one possibility. Another possibility is presented in the flask-sqlalchemy test docs.

[kalbfled]

Using a single test database, I have successfully run all tests in tests/app/communication_item sequentially and in parallel using a new "sample_communication_item" fixture that contains its own teardown. I need to repeat this process for the remaining fixtures and tests.

[kalbfled]

I have refactored the heavily used sample_service, sample_user, and sample_api_key fixtures, including giving them session scope. With these and other changes, the current state of unit tests run locally with 11 threads is:

188 failed, 1192 passed, 147 skipped, 37 xfailed, 2084 errors

This is actually much better than I expected at this point. I'm certain that many of the errors and failures have common causes, and many of the tests are parametrized. Today, I plan to refactor the sample_template and sample_notification fixtures. I expect that to have a significant impact.

[kalbfled]

End of day results:

1113 failed, 1623 passed, 147 skipped, 36 xfailed, 782 errors

This is after more fixtures changes. I have some tests passing individually by failing with "detached instance" when run together sequentially. I will push changes again when I fix that.

[kalbfled]

I have all the tests passing in tests/app/authentication. This involved modifying core fixtures for api keys, services, and users that are used in many other tests.

1089 failed, 1662 passed, 150 skipped, 36 xfailed, 723 errors

Without additional effort, I found that tests pass in tests/app/aws/.

[kalbfled]

I continue to experience unexpected issues during test or fixture teardown. The problem might have something to do with application context during testing.

https://flask-sqlalchemy.palletsprojects.com/en/3.1.x/contexts/#tests

[kalbfled]

After extensive revision to the template and notification fixtures:

374 failed, 1313 passed, 156 skipped, 37 xfailed, 1776 errors

The tests all pass in the tests/app/ folders beginning with "a" and "b", in tests/app/communication_item, and in the tests/app/template* folders. This is progress even though the total number of tests passing dropped.

[k-macmillan]

Current status:
Private Zenhub Image

Passing list:

• accept_invite, api_key, attachments, authentication, aws
• billing (couple teardown issues)
• callback
  • some celery
• va, v3, v2, user, template_statistics, template_folder, template, status
  • some service

[kalbfled]

11 threads

64 failed, 2916 passed, 660 skipped, 55 xfailed, 8 errors

[kalbfled]

11 threads

56 failed, 2924 passed, 660 skipped, 55 xfailed, 9 errors

[kalbfled]

All tests pass when run sequentially! With 11 threads:

65 failed, 2915 passed, 660 skipped, 55 xfailed, 11 errors

There are artifacts in these tables:

• daily_sorted_letter fixed
• va_profile_local_cache

The latter is expected and only affects one suite of lambda function unit tests.

The remaining problems are only apparent when running tests in parallel. For example, run pytest tests/app/dao/test_provider_details_dao.py -q -n11 --tb=no.

[kalbfled]

After marking additional tests as "serial", the status with 9 threads is:

35 failed, 2944 passed, 660 skipped, 55 xfailed, 3 errors

All tests continue to pass when run singled-threaded. I am in the process of evaluating the remaining failures and either fixing them or marking them a "serial" as well. Kyle and I discussed a new ticket for revising "serial" tests for parallel.

[kalbfled]

With 9 threads in parallel + 75 tests run sequentially:

28 failed, 2953 passed, 660 skipped, 55 xfailed, 7 errors

I added the -q flag to run_tests.sh to lessen the output while tests run.

[kalbfled]

With 9 threads in parallel + 80 tests run sequentially:

24 failed, 2954 passed, 660 skipped, 55 xfailed, 4 errors

[kalbfled]

With 9 threads in parallel + 80 tests run sequentially:

19 failed, 2958 passed, 660 skipped, 55 xfailed, 4 errors

[kalbfled]

With 9 threads in parallel:

4 failed, 2896 passed, 660 skipped, 55 xfailed, 1 error

With 107 tests running sequentially:

4 failed, 103 passed, 3589 deselected

Sequential failures:

FAILED tests/app/dao/test_provider_details_dao.py::test_can_get_sms_providers_in_order_of_priority
FAILED tests/app/dao/test_provider_details_dao.py::test_get_current_sms_provider_returns_provider_highest_priority_active_provider
FAILED tests/app/dao/test_provider_details_dao.py::test_switch_sms_provider_to_current_provider_does_not_switch
FAILED tests/app/dao/test_provider_details_dao.py::test_switch_sms_provider_to_inactive_provider_does_not_switch

Parallel failures:

FAILED tests/app/dao/notification_dao/test_notification_dao.py::test_delivery_is_delivery_slow_for_provider_filters_out_notifications_it_should_not_count[options7-False-False]
FAILED tests/lambda_functions/va_profile/test_va_profile_integration.py::test_va_profile_opt_in_out_lambda_handler_new_row
FAILED tests/lambda_functions/va_profile/test_va_profile_integration.py::test_va_profile_opt_in_out_lambda_handler_valid_bytes
FAILED tests/lambda_functions/va_profile/test_va_profile_integration.py::test_va_profile_opt_in_out_lambda_handler_newer_date
ERROR tests/app/dao/test_communication_item_dao.py::TestGetCommunicationItems::test_gets_all_communication_items

[kalbfled]

I just got a clean parallel run, but these tests fail when running sequentially:

FAILED tests/app/dao/test_provider_details_dao.py::test_can_get_email_providers_in_order_of_priority
FAILED tests/app/dao/test_provider_details_dao.py::test_update_adds_history
FAILED tests/app/dao/test_provider_details_dao.py::test_updated_at - sqlalche...
FAILED tests/app/dao/test_provider_details_dao.py::test_dao_get_provider_stats_returns_data_in_type_and_identifier_order
FAILED tests/app/delivery/test_send_to_providers.py::test_should_return_highest_priority_active_provider
========== 5 failed, 137 passed, 3554 deselected in 121.54s

[kalbfled]

All tests pass or are skipped/x-failed. Given the long running nature of this ticket, @k-macmillan and I decided to create new tickets to clean up residual tech debt, etc. See #1631, #1634, and #1635.