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

TST/REF: unify and parametrize data, backend, and client fixtures #116

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

tangkong
Copy link
Contributor

@tangkong tangkong commented Dec 19, 2024

Description

  • Adds test_data, test_backend, and test_client fixtures that can be parametrized, replacing other specialized fixtures that setup specific combinations of daa / backend / client

  • Adds setup_test_stack to concisely parametrize and request the aforementioned test_* fixtures

  • Organizes data-helper functions into a separate file (for my poor scrolling finger)

  • Adjusts existing tests to use setup_test_stack and test_* fixtures instead of other specialized fixtures

  • Fixes bug where TestBackend could not delete an entry that was a direct child of the Root

  • Fixes behavior of FilestoreBackend to throw Entry*Error exceptions rather than just returning None

Motivation and Context

This is the natural evolution of Devan's filestore_backend parametrization efforts (#104) . I know I deleted it but its spirit lives on.

This helps us avoid lots of fixtures that hold a very specific combination of test data, backend type, and client features.

How Has This Been Tested?

By seeing that the test suite still passes

Where Has This Been Documented?

This PR, and overly verbose docstrings

Pre-merge checklist

  • Code works interactively
  • Code follows the style guide
  • Code contains descriptive docstrings, including context and API
  • New/changed functions and methods are covered in the test suite where possible
  • Test suite passes locally
  • Test suite passes on GitHub Actions
  • Ran docs/pre-release-notes.sh and created a pre-release documentation page
  • Pre-release docs include context, functional descriptions, and contributors as appropriate

@tangkong
Copy link
Contributor Author

tangkong commented Dec 19, 2024

This diff count looks crazy but it's mostly that I moved a huge function to another file 😅

@@ -139,8 +141,8 @@ def test_page_smoke(page: str, request: pytest.FixtureRequest):
print(type(request.getfixturevalue(page)))


@pytest.mark.parametrize("filestore_backend", ["db/filestore.json"], indirect=True)
def test_apply_filter(search_page: SearchPage):
@setup_test_stack(sources=["db/filestore.json"], backend_type=FilestoreBackend)
Copy link
Member

Choose a reason for hiding this comment

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

I appreciate how readable these ended up being

Copy link
Member

@ZLLentz ZLLentz left a comment

Choose a reason for hiding this comment

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

I don't think my end-of-day review on holiday brain was thorough enough to click approve but I'm on board
I can return tomorrow and bring a real brain

@tangkong
Copy link
Contributor Author

No rush, I want Devan's input too, and we may have another lively discussion or two about it

@ZLLentz
Copy link
Member

ZLLentz commented Dec 20, 2024

In that case, I'll coast through to eoy and look in the next

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants