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

Snapshot feature with test-cases #983

Merged
merged 8 commits into from
Dec 2, 2023
Merged

Conversation

umututku03
Copy link
Contributor

@umututku03 umututku03 commented Nov 25, 2023

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 and pyta/tests/test_debug/test_accumulation_table.py. The
snapshot.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 the
relevant 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):

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Refactoring (internal change to codebase, without changing functionality)
  • Test update (change that modifies or updates tests only)
  • Documentation update (change that modifies or updates documentation only)
  • Other (please specify):

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

  • I have performed a self-review of my own code.
  • I have verified that the CI tests have passed.
  • I have reviewed the test coverage changes reported on Coveralls.
  • I have added tests for my changes.
  • I have updated the CHANGELOG.md file.

@umututku03 umututku03 marked this pull request as ready for review November 26, 2023 18:51
@umututku03
Copy link
Contributor Author

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.

Copy link
Contributor

@david-yz-liu david-yz-liu left a 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:

  1. Make sure all functions have type annotations.
  2. Make sure all test cases have docstrings.
  3. 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 the in 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 test func1, one case to test func2, and a third test case to test func3.

@coveralls
Copy link
Collaborator

coveralls commented Dec 1, 2023

Pull Request Test Coverage Report for Build 7064669316

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 94.638%

Totals Coverage Status
Change from base Build 6938205699: 0.0%
Covered Lines: 3442
Relevant Lines: 3637

💛 - Coveralls

@david-yz-liu david-yz-liu merged commit 9586a85 into pyta-uoft:master Dec 2, 2023
6 checks passed
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.

3 participants