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

no cache fixtures POC #12816

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

niroshaimos
Copy link

No description provided.

and fixturedef is not None
and fixturedef.use_cache is False
):
self._fixture_defs.pop(argname)
Copy link
Member

Choose a reason for hiding this comment

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

as mentioned in the issue before, i think its important to use a scope name here as indicator instead of the new boolean (in particular as use_cache as implemented here invalidates the scope

the scope name we had in mind back in 2020 was "invocation" to mean that the scope of a validity for a fixture value would be the call (be it other fixture or test function in which it was used

i like the hack with just popping the values as a workaround for the current tech debt as it seems like the impact is limited, but i'd like the input of the others in this

Copy link
Author

Choose a reason for hiding this comment

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

I see.
If we insist on going with the scope like approach rather than the boolean, I think we could still work-around this while leaving the implementation the same. We just change the arg name to a string scope and will currently only support one scope for this (or two considering default behavior).
This way changing the implementation to a more robust infrastructure centered around the actual scoping won't be breaking.
I think this would be a step in the right direction and would probably solve the general use-case for this issue :)
Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

That's the rough idea - add a new scope name, then use the workaround you created and gate on the scope name

For good measure we might need a test that ensures setup/teardown is in sync with the usage sites

Copy link
Author

@niroshaimos niroshaimos Oct 5, 2024

Choose a reason for hiding this comment

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

I've made the rough changes as I understand the request.
I have added a new scope Invocation which will be a scope lower than Function (of course it's cleanup scope will occur during Function cleanup level)
Of course if this is the direction we will be going with I'll move the tests to test_scope.py
When it comes to coverage of the feature, I believe what I have created should be sufficient. I am not sure I understand the additional test suggestions you have provided. If you could expand on it, that would be great.
Please take a look and tell me what you think :)

@niroshaimos niroshaimos force-pushed the no-cache-fixture branch 2 times, most recently from 340f165 to 978085e Compare October 5, 2024 14:05
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