-
Notifications
You must be signed in to change notification settings - Fork 58
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
Snapshot manager #1086
base: master
Are you sure you want to change the base?
Snapshot manager #1086
Conversation
…argument directly
Pull Request Test Coverage Report for Build 11110823253Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@leowrites good work. In this first review, I focused on the API of SnapshotManager
as well as some high-level comments to improve the test file. Make sure to also update the documentation accordingly, which I'll review more closely after you've addressed these comments.
SNAPSHOT_DIR = os.path.join( | ||
os.path.dirname(os.path.realpath(__file__)), "snapshot_manager_testing_snapshots" | ||
) | ||
TEST_RESULTS_DIR = "/tmp/test_results" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pytest provides a fixture for creating a temporary directory, use that instead of defining your own here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I defined a directory here because the output directory is captured in the snapshots, so we want the directories to be consistent across each test run.
I looked into using tmp_path
and tmp_path_factory
but I can't seem to get the same base output directory for each run (an example directory I got is /private/var/folders/rt/x1pynv8j1fb1hkbhs6y9v62m0000gn/T/pytest-of-leoliu/pytest-0
). The only way I found to ensure the base directory is the same is to set the --basetemp
. Should I rely on this flag to ensure the directories are consistent?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@leowrites that's a great question. This by itself suggests a limitation with the tests, because (of course) you mean to only test the part inside the with
block, and not this output_directory
parameter.
I think you should be able to change your function structure by nesting the "function input" definition within the test case itself, making the output_directory
variable non-local to the function. Something like:
def test_func_one_line(tmp_dir)
output_directory = ... # tmp_dir's path
def func_one_line():
with SnapshotTracer(...):
num = 123
func_one_line()
assert_output_files_match(..., ...)
This loses the nice use of pytest.parameterize
that you have, but I think overall the code complexity stays almost the same because you move the function input definitions into the test case code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@david-yz-liu By nesting the functions, the output_directory
is still captured in f_locals
inside func_one_line
. I think this is because f_locals
also includes free variables in the parent scope when they are referenced. One solution I found is using a global variable so that the variable won't show up in f_locals
, but it feels unconventional and could break if we were to run the tests concurrently
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@leowrites wow, yeah okay. I'm not sure how I feel about free variables appearing the stack frame, but I get why it's happening.
All that said, it's not good to use an absolute path (can't assume specific permissions) and it's almost certainly going to look different on Windows.
Why don't we take a pause on this and introduce a different approach. One snapshot
feature that's on our roadmap is to allow the user to exclude certain variables (again, as an iterable string/regex). It's a different kind of filtering, I suppose, now at the individual frame level. If you implement that, you'll be able to use it here to exclude output_directory
from the memory model diagram
Proposed Changes
python_ta.debug.SnapshotManager
Type of Change
(Write an
X
or a brief description next to the type or types that best describe your changes.)Checklist
(Complete each of the following items for your pull request. Indicate that you have completed an item by changing the
[ ]
into a[x]
in the raw text, or by clicking on the checkbox in the rendered description on GitHub.)Before opening your pull request:
After opening your pull request:
Questions and Comments
(Include any questions or comments you have regarding your changes.)