bloomberg / pytest-memray

pytest plugin for easy integration of memray memory profiler
https://pytest-memray.readthedocs.io/en/latest/
Apache License 2.0
354 stars 25 forks source link

Long test names can be problematic #68

Closed pablogsal closed 1 year ago

pablogsal commented 1 year ago

See https://github.com/pydantic/pydantic/pull/5052#issuecomment-1430841915

marcaube commented 1 year ago

We also encounter this issue with many tests in one of our projects. We err on being overly explicit in our test names, combined with verbose scenario names in @pytest.mark.parametrize, which often take us well over the filename length limit.

Are the binary dumps strictly necessary for memray to work? One workaround could be a CLI flag to disable them, what do you think?

pablogsal commented 1 year ago

Are the binary dumps strictly necessary for memray to work?

Unfortunately yes for the time being, but in the next release, we will have the possibility to aggregate in memory so we may be able to offer the CLI flag you mention.

We could also try to limit the test names with some hashing to help with this problem

marcaube commented 1 year ago

Another option that may not be ideal, but worked in our case, was truncating the file names to 200-something chars and, accounting for hash and extension, left us under the limit.

Edit: we monkey patch memray for now.

godlygeek commented 1 year ago

@marcaube Are you providing the --memray-bin-path argument when you launch pytest?

From what I recall, and what I can tell by testing, we don't give an error message when a test name is too long when you run pytest --memray, but we do when you run pytest --memray --memray-bin-path some_dir/

But if that's the case, you've explicitly asked for Memray to leave well-named capture files around for you to inspect after the tests have run, in which case your question:

Are the binary dumps strictly necessary for memray to work? One workaround could be a CLI flag to disable them

doesn't make much sense to me.

Can you elaborate a bit on how you're invoking pytest?

marcaube commented 1 year ago

@godlygeek It happens whether or not I provide the --memray-bin-path argument. Simply adding --memray to my pytest command triggers a slew of OSError: [Errno 63] File name too long errors.

CleanShot 2023-06-05 at 12 36 05@2x

godlygeek commented 1 year ago

Interesting... That isn't what I would have expected, and I wasn't able to reproduce that from some quick testing.

Hm. When _tmp_dir is not None, we use a randomly generated name for the capture file, with no connection to the test name...

https://github.com/bloomberg/pytest-memray/blob/20dee74441772c272bcfbc362c32826d82b88c3d/src/pytest_memray/plugin.py#L137-L144

There's only one place where we set it to non-None:

https://github.com/bloomberg/pytest-memray/blob/20dee74441772c272bcfbc362c32826d82b88c3d/src/pytest_memray/plugin.py#L91-L104

And that's when neither --memray-bin-path was passed nor the MEMRAY_RESULT_PATH environment variable is set. MEMRAY_RESULT_PATH is set when pytest-xdist is in use, and your screenshot shows that the xdist-3.3.1 plugin is loaded and used. So OK, that's one mystery solved, at least...

I think we could probably do something about this. There's no particular reason why the files must be named based on the test name when pytest-xdist is in use. The only requirement is that we must be able to pair them up with the test they correspond to after the fact. We could perhaps do that by numbering the tests at collection time, and just giving the files monotonically increasing numbers as names, or something like that...

godlygeek commented 1 year ago

@marcaube The fix just landed in #77 should fix this for you, I believe, and bring us back to the state I thought were were in already, where this error would only occur if --memray-bin-path is specified.

marcaube commented 1 year ago

Woot! Awesome, I'm going to try that tomorrow morning 👏

pablogsal commented 1 year ago

@marcaube I am closing this issue, but please feel free to open if there is anything missing