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

Report memory limit and allocated memory in longrepr #5

Merged
merged 8 commits into from
May 16, 2022

Conversation

petr-tik
Copy link
Contributor

@petr-tik petr-tik commented Apr 23, 2022

Make the longrepr more descriptive and enrich the xml output with
this information.

Refactor limit_memory to return a _MemoryInfo type, which
can be presented as a PytestSection and a longrepr string.

Remove item.nodeid from longrepr since longrepr is only used by
junit-xml parser and the structure of the xml document
makes the item.nodeid in the error message redundant.

Testing performed
Add a test that parses the resulting xml file to make sure
the expected string was written.

Issue number of the reported bug or feature request: #3

Refactor limit_memory to return a schema-based type, which
can be converted to a PytestSectio and a longrepr string.

Remove item.nodeid from longrepr since longrepr is only used by
junit-xml parser and the structure of the xml document
makes the item.nodeid in the error message redundant.

Add a test that parses the resulting xml file to make sure
the expected string was written.
@gaborbernat
Copy link
Contributor

@petr-tik can you try to resolve the merge conflicts?

@petr-tik petr-tik marked this pull request as ready for review May 15, 2022 18:05
@petr-tik
Copy link
Contributor Author

@gaborbernat i have. please take a look when you can

tests/test_pytest_pensieve.py Outdated Show resolved Hide resolved
tests/test_pytest_pensieve.py Show resolved Hide resolved
tests/test_pytest_pensieve.py Outdated Show resolved Hide resolved
src/pytest_memray/marks.py Outdated Show resolved Hide resolved
src/pytest_memray/marks.py Outdated Show resolved Hide resolved
src/pytest_memray/marks.py Outdated Show resolved Hide resolved
src/pytest_memray/marks.py Outdated Show resolved Hide resolved
src/pytest_memray/marks.py Outdated Show resolved Hide resolved
src/pytest_memray/marks.py Outdated Show resolved Hide resolved
Convert both FailedTestMemoryInfo methods into properties

Regularise indentation, code style, explicit None returns, type hints
@petr-tik
Copy link
Contributor Author

Thanks for the detailed review with code style and refactor suggestions.

Addressed all of them. Only have 1 REVIEW comment about the name of the dataclass, happy to remove the comment, if you find the type name descriptive enough.

src/pytest_memray/marks.py Outdated Show resolved Hide resolved
src/pytest_memray/marks.py Outdated Show resolved Hide resolved
tests/test_pytest_pensieve.py Show resolved Hide resolved
Signed-off-by: Bernát Gábor <bgabor8@bloomberg.net>
Copy link
Contributor

@gaborbernat gaborbernat left a comment

Choose a reason for hiding this comment

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

@petr-tik
Copy link
Contributor Author

do you want me to edit the PR message before merging?

@gaborbernat
Copy link
Contributor

If you have any amendments you think makes sense, sure 👍

@petr-tik
Copy link
Contributor Author

i wanted to tidy it up and leave a descriptive commit message in main

@gaborbernat
Copy link
Contributor

Go ahead with that then 👍

@petr-tik petr-tik changed the title Change longrepr report to include memory info Report memory limit and allocated memory in longrepr May 16, 2022
@petr-tik
Copy link
Contributor Author

Done. I am guessing you are the one with squash and merge rights?

@pablogsal pablogsal merged commit 16b256c into bloomberg:main May 16, 2022
@pablogsal
Copy link
Member

Thanks a lot @petr-tik for the fantastic work and for persuing getting this fixed ❤️

And thanks a lot @gaborbernat for the review! 💪

@petr-tik
Copy link
Contributor Author

oh, my PR message wasn't included in the commit.

Thanks for the detailed review. Absolute pleasure

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.

None yet

3 participants