-
Notifications
You must be signed in to change notification settings - Fork 64
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 feature with test-cases #983
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Hello @david-yz-liu ! I acknowledge the existing conflicts and eagerly await your review and feedback on additional issues. Once received, I'll promptly address and resolve them collectively. |
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.
@umututku03 overall this looks great. Some overall comments:
- Make sure all functions have type annotations.
- Make sure all test cases have docstrings.
- You can improve the test cases as follows: each test case should only have one call to
func1
/func2
/func3
, and you should store the result in a variable. Then instead of using thein
operator, you should check specific indexes and assert that the stack frames appear in the expected order. Hopefully this helps clarify why I want one test case to testfunc1
, one case to testfunc2
, and a third test case to testfunc3
.
Pull Request Test Coverage Report for Build 7064669316
💛 - Coveralls |
david-yz-liu
approved these changes
Dec 2, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation and Context
Currently, the input for generating memory model diagrams involves manual creation based on provided instructions. This manual process is prone to errors and consumes a significant amount of time. To address this challenge, we aim to automate the creation of memory model diagrams by deriving a list of dictionaries containing local variables from relevant functions or stack frames where the diagrams are needed. The resulting list will then serve as an input for streamlining the automation process, enhancing accuracy, and saving valuable time.
Your Changes
I have made changes in two modules:
pyta/debug/snapshot.py
andpyta/tests/test_debug/test_accumulation_table.py
. Thesnapshot.py
file is a brand-new file created where our implementation for capturing a "snapshot" of local variables from the current and outer stack frames where the 'snapshot' function is called. I have also included a variety of test cases in therelevant test modules. For relevant documentation, see the files.
Description:
I have used the 'inspect' module to extract local variables from multiple stack frames without relying on the built-in
locals function. Also, I have ensured that the behaviour of irrelevant stack-frames are not recorded.
Type of change (select all that apply):
Testing
I have several unit tests to test the behaviour of our implementation. I have included tests
for a single function (i.e. no level of nesting). Also, nested functions (multiple level of nested
structure) are tested, and it's observed whether the local variables of all these functions are
obtained.
Questions and Comments (if applicable)
Is there any suggestions for naming conventions for unit tests? I feel like I am being unprofessional
for them, which I am not sure whether it's relevant or not in our case.
Checklist