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: parametrize the filestore_backend fixture #104

Merged

Conversation

shilorigins
Copy link
Collaborator

@shilorigins shilorigins commented Nov 14, 2024

Description

Refactor the filestore_backend fixture to accept parametrized args. Each arg can be a path to a file or an iterable of functions or function names. Function names must be accessible from the conftest.py namespace. Functions must return a Root or an iterable of Entrys.

Closes #103

Motivation and Context

Parametrizing filestore_backend enables us to choose what data to use in a test without having to define a dedicated fixture. Parametrization also allows us to easily run the same test on multiple data sources.

Supporting function names in the conftest.py namespace let's us select widely used data sources defined in conftest.py without having to import the functions. Supporting function literals additionally let's us define more narrowly used data sources in test files and pass those into the backend.

How Has This Been Tested?

Existing tests were adjusted to specify the same data they were using before. I also added a test that uses function names as the parameter; this test will be replaced in an upcoming PR.

Where Has This Been Documented?

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

@shilorigins shilorigins self-assigned this Nov 14, 2024
@shilorigins shilorigins changed the title Devagr/parametrized backend fixture TST: parametrize the filestore_backend fixture Nov 14, 2024
@shilorigins shilorigins force-pushed the devagr/parametrized-backend-fixture branch from bbb311d to 31181d5 Compare November 14, 2024 21:20
@shilorigins
Copy link
Collaborator Author

I haven't cleaned up the code yet, but this is functional in it's current state. Let me know if you have any suggestions about what we should or shouldn't support.

@shilorigins shilorigins force-pushed the devagr/parametrized-backend-fixture branch from 31181d5 to 18a9fd1 Compare November 15, 2024 23:30
The intermediate utility function populate_backend is also used to simplify
bin.demo::main
@shilorigins shilorigins force-pushed the devagr/parametrized-backend-fixture branch from 18a9fd1 to d65e4af Compare November 15, 2024 23:38
@shilorigins shilorigins marked this pull request as ready for review November 15, 2024 23:41
Copy link
Contributor

@tangkong tangkong left a comment

Choose a reason for hiding this comment

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

A couple of comments, I like the idea of this but I think the way we invoke the parametrization is confusing. I may be misunderstanding things, let me know if I'm off base

superscore/tests/conftest.py Show resolved Hide resolved
superscore/tests/conftest.py Outdated Show resolved Hide resolved
superscore/tests/test_backend.py Show resolved Hide resolved
superscore/tests/test_page.py Show resolved Hide resolved
@tangkong
Copy link
Contributor

Have you thought about how we might also parametrize the client fixtures (to maybe take any arbitrary parametrized backend)

That's the natural extension of this, and I'm unsure if the current fixture-shadowing method would extend to that

@shilorigins
Copy link
Collaborator Author

In this week's design discussion, we worked through the parametrization paradigm and how the fixtures interact. I believe decided the current version is acceptable with a docstring explaining the intended use, but it may change as part of a refactor to make clients or backend types similarly parametrizatable. Let me know if this is satisfactory.

@shilorigins shilorigins requested a review from tangkong November 21, 2024 00:07
Copy link
Contributor

@tangkong tangkong left a comment

Choose a reason for hiding this comment

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

One very healthy discussion later, I think I'm ok with this knowing that there's yet more test suite refactoring to do. I do believe there should be a way to parametrize the client / backend / datasource fixtures while minimizing calls to the component fixtures, but that's going to require a bit more invasive refactoring in any case.

For now I think this accomplishes the goal of making our specific filestore fixture more flexible, while also laying the groundwork for future efforts

@shilorigins shilorigins merged commit fc867b5 into pcdshub:master Nov 21, 2024
11 checks passed
@shilorigins shilorigins deleted the devagr/parametrized-backend-fixture branch November 21, 2024 22:54
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.

TST: enable creating Client / Backend with parametrized data
3 participants