Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#1436 refactor unit tests to use a single test database #1629

Closed
wants to merge 6 commits into from

Conversation

kalbfled
Copy link
Member

@kalbfled kalbfled commented Feb 5, 2024

Description

Refactor unit tests to use a single test database. These changes are extensive but follow a few themes:

  • Remove all uses of the worker_id fixture, which provides a unique identifier for each test worker.
  • Overhaul fixtures to use the factory paradigm, including appropriate tear-down, and use this paradigm in all tests.
  • Where, prudent, replace legacy SQLAlchemy query syntax with 2.0 syntax. (The ticket for upgrading SQLAlchemy to v2 will finish this process.)
  • Avoid IntegrityError (unique constraint violations, for example) by not using hard-coded values for service names, etc.
  • Modify some test assertions that check the number of rows returned by a query. These fail for parallel execution because other tests could have created rows unrelated to the current test.

There are several follow-up tickets associated with this work, as noted in the comments on the ticket.

#1436

Type of change

Please check the relevant option(s).

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • Documentation changes only

How Has This Been Tested?

How has this been tested? (e.g. Unit tests, Tested locally, Tested as a GitHub action, Depoyed to dev, etc)
Please provide relevant information and / or links to demonstrate the functionality or fix. (e.g. screenshots, link to deployment, regression test results, etc)

Checklist

  • I have assigned myself to this PR
  • PR has an appropriate title: #9999 - What the thing does
  • PR has a detailed description, including links to specific documentation
  • I have added the appropriate labels to the PR.
  • I did not remove any parts of the template, such as checkboxes even if they are not used
  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to any documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have ensured the latest master is merged into my branch and all checks are green prior to review
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • The ticket is now moved into the DEV test column

Co-authored-by: Kyle MacMillan <[email protected]>
Co-authored-by: Michael Wellman <[email protected]>
@kalbfled kalbfled marked this pull request as ready for review February 6, 2024 19:29
@kalbfled kalbfled requested a review from a team as a code owner February 6, 2024 19:29

# TODO - Catch db connection errors and retry?
with get_reader_session() as reader_session:
query = select(Template).where(Template.id == request_data['template_id'])
try:
# TODO - This should intead use reader_session.get which returns an object or None

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo in instead
TODOs need ticket numbers. Please add one.

@@ -196,6 +188,7 @@ def v3_send_email_notification(
# Persist the notification so related model instances are available to downstream code.
notification.status = NOTIFICATION_CREATED
db.session.add(notification)
# TODO - Is this necessary? The template isn't being modified. Refreshing fails.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODOs need ticket numbers. Please add it.

@kalbfled kalbfled marked this pull request as draft February 6, 2024 20:34
@kalbfled
Copy link
Member Author

kalbfled commented Feb 6, 2024

Superceeded by PR 1633.

@kalbfled kalbfled closed this Feb 6, 2024
@k-macmillan k-macmillan deleted the 1436-2 branch November 29, 2024 18:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants