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.

Sign up for GitHub

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

Jump to bottom

Update sys.path when running memray run #86

Merged
merged 2 commits into from May 16, 2022
Merged

Update sys.path when running memray run #86

merged 2 commits into from May 16, 2022

Conversation

tal66
Copy link
Contributor

@tal66 tal66 commented May 8, 2022

Hi, this closes #76.

  • refactor suggestion to test_cli.py::TestRunSubCommand (class level mocks limited testing possibilities)
  • added test that fails when not editing sys.path
  • added method edit_sys that edits sys.path (also moved existing sys edits to it).
    sys.path[0] will be the directory containing args.script, or '.' when run -c (like python does)

@godlygeek
tests: Test run's sys.path/sys.argv[0] updates
802b8f4 
In order to match the behavior of `python foo.py`, `python -m foo`, and
`python -c "foo"`, `memray run` manipulates `sys.path` and `sys.argv[0]`
to match the modifications that the interpreter itself would do.

Add tests validating that the modifications work as expected.

Signed-off-by: Matt Wozniski <mwozniski@bloomberg.net>
@tal66 @godlygeek
run: Edit sys.path before running args.script
fb510a2 
This enables finding local modules. Also, ensure that `sys.argv[0]`
matches what it would if the user had run this command without Memray,
to minimize the observeable differences our runner forces on users'
scripts.

Signed-off-by: tal66 <77445020+tal66@users.noreply.github.com>
Signed-off-by: Matt Wozniski <mwozniski@bloomberg.net>
@godlygeek
Copy link
Contributor

Thanks @tal66 - I picked this up, but ran in a bit of a different way with it. In particular, the reason you were struggling with mocks in the tests was that you were putting the new tests into the wrong layer - the tests in the unit/ directory are supposed to have no external dependencies, which is why so many things were being mocked out.

The tests that we need in order to validate that this is working properly fit better into the integration/ directory, where we can do a full run of the memray run command on a file or module that we control and observe how the actual command behaves when run as a subprocess, so I've updated to do that.

@pablogsal Since I've made some pretty extensive changes here, I'd rather not land this myself - would you mind giving this a review?

@godlygeek godlygeek changed the title Edit sys path Update sys.path when running memray run May 16, 2022
@pablogsal
Copy link
Member

Thanks for adding tests for all three cases :)

@pablogsal pablogsal merged commit 348b74f into bloomberg:main May 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pablogsal pablogsal pablogsal approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Memray seems to modify sys.path such that local module imports fail
3 participants
@tal66 @godlygeek @pablogsal